Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable resultset validation of Drill tests #15096

Merged
merged 129 commits into from
Oct 10, 2023

Conversation

kgyrtkirk
Copy link
Member

  • introduces a test_X method for every testcase (995 testcases)
  • added a resultset parser which reads the expected resultset based on the result schema
  • loaded a few more datasets
  • added a testcase to ensure that all files have a corresponding testcase
  • renamed DecoupledIgnore to NegativeTest
  • categorized the failing 268 tests

current histogram of failures:

grep '@NegativeTest' sql/src/test/java/org/apache/druid/sql/calcite/DrillWindowQueryTest.java|sort|uniq -c|sort -n
      1   @NegativeTest(Modes.ALLDATA_CSV)
      1   @NegativeTest(Modes.BIGINT_TIME_COMPARE)
      1   @NegativeTest(Modes.CANNOT_APPLY_VIRTUAL_COL)
      1   @NegativeTest(Modes.COLUMN_NOT_FOUND)
      1   @NegativeTest(Modes.INCORRECT_SYNTAX)
      1   @NegativeTest(Modes.NPE_PLAIN)
     12   @NegativeTest(Modes.MISSING_DESC)
     18   @NegativeTest(Modes.BIGINT_TO_DATE)
     23   @NegativeTest(Modes.AGGREGATION_NOT_SUPPORT_TYPE)
     25   @NegativeTest(Modes.NOT_ENOUGH_RULES)
     36   @NegativeTest(Modes.NPE)
     41   @NegativeTest(Modes.RESULT_COUNT_MISMATCH)
    107   @NegativeTest(Modes.NULLS_FIRST_LAST)

This PR has:

  • been self-reviewed.

@kgyrtkirk kgyrtkirk marked this pull request as ready for review October 9, 2023 18:50
List<Object[]> expectedResults = parseResults(currentRowSignature, expectedResultsText);
try {
Assert.assertEquals(StringUtils.format("result count: %s", sql), expectedResultsText.size(), results.size());
if (!isOrdered(sql)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this be flipped?

Copy link
Member Author

Choose a reason for hiding this comment

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

the logic here is kinda like: if the resultset is not ordered - lets order both of them by the same comparator and expect the same (ordered) resultsets after that

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah ok. Because there is no guarantee about what order the results will arrive in. can you please add a comment here in the code itself?

Copy link
Contributor

Choose a reason for hiding this comment

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

can be done in a follow-up PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've changed a few more things - and made SqlToRelConverter#isOrdered do the decision here - but these changes become dependendt on #15086 ; I can add it after that

@abhishekagarwal87 abhishekagarwal87 merged commit 23605c1 into apache:master Oct 10, 2023
81 checks passed
@abhishekagarwal87 abhishekagarwal87 added this to the 28.0 milestone Oct 10, 2023
pranavbhole pushed a commit to pranavbhole/druid that referenced this pull request Oct 12, 2023
- introduces a test_X method for every testcase (995 testcases)
- added a resultset parser which reads the expected resultset based on the result schema
- loaded a few more datasets
- added a testcase to ensure that all files have a corresponding testcase
- renamed DecoupledIgnore to NegativeTest
- categorized the failing 268 tests
@pranavbhole pranavbhole mentioned this pull request Oct 12, 2023
10 tasks
pranavbhole pushed a commit to pranavbhole/druid that referenced this pull request Oct 12, 2023
- introduces a test_X method for every testcase (995 testcases)
- added a resultset parser which reads the expected resultset based on the result schema
- loaded a few more datasets
- added a testcase to ensure that all files have a corresponding testcase
- renamed DecoupledIgnore to NegativeTest
- categorized the failing 268 tests
LakshSingla pushed a commit that referenced this pull request Oct 13, 2023
- introduces a test_X method for every testcase (995 testcases)
- added a resultset parser which reads the expected resultset based on the result schema
- loaded a few more datasets
- added a testcase to ensure that all files have a corresponding testcase
- renamed DecoupledIgnore to NegativeTest
- categorized the failing 268 tests
ektravel pushed a commit to ektravel/druid that referenced this pull request Oct 16, 2023
- introduces a test_X method for every testcase (995 testcases)
- added a resultset parser which reads the expected resultset based on the result schema
- loaded a few more datasets
- added a testcase to ensure that all files have a corresponding testcase
- renamed DecoupledIgnore to NegativeTest
- categorized the failing 268 tests
CaseyPan pushed a commit to CaseyPan/druid that referenced this pull request Nov 17, 2023
- introduces a test_X method for every testcase (995 testcases)
- added a resultset parser which reads the expected resultset based on the result schema
- loaded a few more datasets
- added a testcase to ensure that all files have a corresponding testcase
- renamed DecoupledIgnore to NegativeTest
- categorized the failing 268 tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants