Skip to content

SQL: [Tests] Add integ tests for selecting a literal and an aggregate#42121

Merged
matriv merged 1 commit intoelastic:masterfrom
emasab:bugfix/41411-selecting-a-literal
Feb 7, 2020
Merged

SQL: [Tests] Add integ tests for selecting a literal and an aggregate#42121
matriv merged 1 commit intoelastic:masterfrom
emasab:bugfix/41411-selecting-a-literal

Conversation

@emasab
Copy link
Copy Markdown
Contributor

@emasab emasab commented May 13, 2019

No description provided.

@emasab
Copy link
Copy Markdown
Contributor Author

emasab commented May 13, 2019

Closes #41411

@tvernum tvernum added the :Analytics/SQL SQL querying label May 15, 2019
@elasticmachine
Copy link
Copy Markdown
Collaborator

elasticmachine commented May 15, 2019

Pinging @elastic/es-search

@tvernum
Copy link
Copy Markdown
Contributor

tvernum commented May 15, 2019

@elasticmachine test this please

@emasab
Copy link
Copy Markdown
Contributor Author

emasab commented May 18, 2019 •
edited
Loading

... otherwise this query was failing because the literal with this commit is supportedByAggsOnlyQuery

SELECT DAY_NAME(birth_date + INTERVAL 1 DAY) FROM test_emp

@emasab emasab changed the title bugfix for https://github.com/elastic/elasticsearch/issues/41411 SQL: selecting a literal and an aggregate generates an error Jun 10, 2019
@emasab emasab force-pushed the bugfix/41411-selecting-a-literal branch from 996302d to a8986fb Compare June 12, 2019 16:19
@emasab
Copy link
Copy Markdown
Contributor Author

emasab commented Sep 15, 2019 •
edited
Loading

the latest commit fixes some integration tests that were failing. Pending to add more unit/integration tests, at the moment there is only one unit test for the issue that is addressing

In a previous version that I've not sent I had left both supportedByAggsQuery and supportedByNoAggsQuery but given that the not aggregated fields inside an aggregation query are detected in the verification phase, another check should not be necessary. But if you want to maintain it, I could restore. The check was like:

    public boolean isAggsOnly() {
        if (aggsOnly == null) {
            aggsOnly = Boolean.valueOf(this.fields.stream().allMatch(t -> t.v1().supportedByAggsQuery())) &&
!Boolean.valueOf(this.fields.stream().allMatch(t -> t.v1().supportedByNoAggsQuery()));
        }

        return aggsOnly.booleanValue();
    }

in that case the ConstantInput was the only that was supportedByAggsQuery and supportedByNoAggsQuery. The integration tests were successful too with that approach.

@tvernum
Copy link
Copy Markdown
Contributor

tvernum commented Sep 18, 2019

@elastic/es-sql Can someone look at this community PR?

@emasab emasab force-pushed the bugfix/41411-selecting-a-literal branch from 0bd8e98 to 4b2e482 Compare September 20, 2019 18:26
@emasab
Copy link
Copy Markdown
Contributor Author

emasab commented Sep 20, 2019

rebased on current master

@emasab emasab force-pushed the bugfix/41411-selecting-a-literal branch from 4b2e482 to 17721c7 Compare September 25, 2019 21:04
@emasab emasab force-pushed the bugfix/41411-selecting-a-literal branch from 17721c7 to 311ff60 Compare November 1, 2019 20:29
@emasab
Copy link
Copy Markdown
Contributor Author

emasab commented Nov 1, 2019

rebased

@emasab emasab force-pushed the bugfix/41411-selecting-a-literal branch from 311ff60 to f5a07cc Compare November 10, 2019 10:34
@emasab
Copy link
Copy Markdown
Contributor Author

emasab commented Nov 10, 2019 •
edited
Loading

rebased, some formatting and other integration tests

@emasab emasab force-pushed the bugfix/41411-selecting-a-literal branch from 788eefa to e683dc4 Compare November 19, 2019 18:56
@emasab
Copy link
Copy Markdown
Contributor Author

emasab commented Nov 19, 2019

rebased

@matriv
Copy link
Copy Markdown
Contributor

matriv commented Feb 4, 2020

Hi @emasab

Apologies for the delay here. Since the merging of #49570 seems that the issue is solved.
It would be great though to add the tests that you implemented to have this behaviour with
literals and aggregations tested thoroughly. Would you mind adjusting your PR to only add the
new tests?

Thank you!

@matriv matriv mentioned this pull request Feb 4, 2020
Merged
@emasab
Copy link
Copy Markdown
Contributor Author

emasab commented Feb 5, 2020

Hi @matriv, happy to hear from you! I'm going to do that asap.

Best

@matriv
Copy link
Copy Markdown
Contributor

matriv commented Feb 5, 2020

@emasab Thank you!

@emasab emasab force-pushed the bugfix/41411-selecting-a-literal branch 2 times, most recently from 57e3436 to 26283b6 Compare February 6, 2020 23:02
@emasab
Copy link
Copy Markdown
Contributor Author

emasab commented Feb 6, 2020

That's it! Only the tests are left and they're passing.

matriv
matriv approved these changes Feb 6, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@matriv matriv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, left a comment regarding the unit test.

x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryFolderTests.java Outdated
assertThat(a, containsString("{\"avg\":{\"field\":\"int\"}"));
}

public void testFoldingOfLiteralWithGroupBy() {
Copy link
Copy Markdown
Contributor

@matriv matriv Feb 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can remove this, as it's already added here:

elasticsearch/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryFolderTests.java

Line 383 in 26283b6

p = plan("SELECT 1, count(*) FROM test GROUP BY int");

@matriv
Copy link
Copy Markdown
Contributor

matriv commented Feb 6, 2020

@astefan Could you please also take a look?

@emasab emasab force-pushed the bugfix/41411-selecting-a-literal branch from 26283b6 to 61a4700 Compare February 7, 2020 15:30
@emasab
Copy link
Copy Markdown
Contributor Author

emasab commented Feb 7, 2020

@matriv I've removed the duplicated unit test

@matriv matriv added >test-failure Triaged test failures from CI v7.6.1 v7.7.0 >test Issues or PRs that are addressing/adding tests v8.0.0 and removed >test-failure Triaged test failures from CI labels Feb 7, 2020
astefan
astefan approved these changes Feb 7, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@astefan astefan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@matriv matriv changed the title SQL: selecting a literal and an aggregate generates an error SQL: [Tests] Add integ tests for selecting a literal and an aggregate Feb 7, 2020
@matriv
Copy link
Copy Markdown
Contributor

matriv commented Feb 7, 2020

@elasticmachine ok to test

@matriv matriv merged commit 9f414a8 into elastic:master Feb 7, 2020
@matriv
Copy link
Copy Markdown
Contributor

matriv commented Feb 7, 2020

@emasab Thanks a lot for your patience and your contribution!

matriv pushed a commit that referenced this pull request Feb 7, 2020
@emasab @matriv
SQL: [Tests] Add integ tests for selecting a literal and an aggregate (…
282e919 
…#42121)

The related issue regarding aggregation queries where some literals
are also selected together with aggregate function has been fixed
with #49570. Add integration tests to verify the behavior.

Relates to: #41411

(cherry picked from commit 9f414a8)
matriv pushed a commit that referenced this pull request Feb 7, 2020
@emasab @matriv
SQL: [Tests] Add integ tests for selecting a literal and an aggregate (…
e717f41 
…#42121)

The related issue regarding aggregation queries where some literals
are also selected together with aggregate function has been fixed
with #49570. Add integration tests to verify the behavior.

Relates to: #41411

(cherry picked from commit 9f414a8)
matriv added a commit to matriv/elasticsearch that referenced this pull request Feb 7, 2020
@matriv
SQL: [Tests] Add more tests for aggs and literals
953c741 
Add some more tests where more than one literal is selected,
unaliased and aliased.

Follows: elastic#42121
@matriv matriv mentioned this pull request Feb 7, 2020
Merged
matriv added a commit that referenced this pull request Feb 9, 2020
@matriv
SQL: [Tests] Add more tests for aggs and literals (#52086)
405271d 
Add some more tests where more than one literal is selected,
unaliased and aliased.

Follows: #42121
matriv added a commit that referenced this pull request Feb 9, 2020
@matriv
SQL: [Tests] Add more tests for aggs and literals (#52086)
3e7f939 
Add some more tests where more than one literal is selected,
unaliased and aliased.

Follows: #42121
(cherry picked from commit 405271d)
matriv added a commit that referenced this pull request Feb 9, 2020
@matriv
SQL: [Tests] Add more tests for aggs and literals (#52086)
7326708 
Add some more tests where more than one literal is selected,
unaliased and aliased.

Follows: #42121
(cherry picked from commit 405271d)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@astefan astefan astefan approved these changes

@costin costin Awaiting requested review from costin

+1 more reviewer

@matriv matriv matriv approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

:Analytics/SQL SQL querying >test Issues or PRs that are addressing/adding tests v7.6.1 v7.7.0 v8.0.0-alpha1

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@emasab @elasticmachine @tvernum @matriv @astefan @jakelandis @polyfractal