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.

Sign up for GitHub

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

Jump to bottom

Enable already passing tests in DecoupledPlanningCalciteQueryTest #14996

Merged
merged 7 commits into from
Sep 20, 2023
Merged

Enable already passing tests in DecoupledPlanningCalciteQueryTest #14996

merged 7 commits into from
Sep 20, 2023

Conversation

kgyrtkirk
Copy link
Member

There are some tests which were disabled - however they seem to be able to pass already.

@kgyrtkirk kgyrtkirk marked this pull request as ready for review September 15, 2023 18:16
@LakshSingla
Copy link
Contributor

What change caused the test to pass? If it was passing from the beginning then what was the rationale behind disabling them? (cc @rohangarg)

@kgyrtkirk
Copy link
Member Author

@LakshSingla I'm not sure if that's really important
they can be easily overlooked as there is no failure in case they would silently start passing...

the mechanism employed here could be replaced with something which will fail if they start passing for some reason...
so that the same will not happen in the future

do you think it would be usefull to dig out and bisect when they started passing?

@kgyrtkirk
Copy link
Member Author

kgyrtkirk commented Sep 18, 2023 •
edited
Loading

there are a few which was silently fixed by the 1.35 calcite upgrade:

testGroupBySortPushDown
testGroupingWithNullInFilter
testNestedGroupBy
testQueryWithSelectProjectAndIdentityProjectDoesNotRename
testSortProjectAfterNestedGroupBy
testSubqueryTypeMismatchWithLiterals
testTimeseriesQueryWithEmptyInlineDatasourceAndGranularity

and testStringAggExpressionNonConstantSeparator was able to pass when this suite was introduced ; most likely some last minute changes made it pass...

This was referenced Sep 18, 2023
Merged
Merged
@kgyrtkirk
Copy link
Member Author

I think we might want to use a better way to keep this tests disabled until we fix them: #15005

rohangarg
rohangarg approved these changes Sep 20, 2023
View reviewed changes
Copy link
Member

@rohangarg rohangarg left a comment

Choose a reason for hiding this comment

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

Thanks for the changes @kgyrtkirk - we can merge this and you can rebase the new changes in #15005

@rohangarg rohangarg merged commit e8773f4 into apache:master Sep 20, 2023
@kgyrtkirk
Copy link
Member Author

Thank you @rohangarg for reviewing and merging these changes :)
I'll rebase that right away :D

abhishekagarwal87 pushed a commit that referenced this pull request Sep 29, 2023
@kgyrtkirk
Enable SortProjectTransposeRule (#15002)
db71e28 
contains Enable already passing tests in DecoupledPlanningCalciteQueryTest #14996
enables a transpose rule to support a query plan in which the plan was in the shape:
Sort
  Project
     Aggregate
@LakshSingla LakshSingla added this to the 28.0 milestone Oct 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rohangarg rohangarg rohangarg approved these changes

Assignees
No one assigned
Labels
Area - Querying
Projects
None yet
Milestone
28.0
Development

Successfully merging this pull request may close these issues.
3 participants
@kgyrtkirk @LakshSingla @rohangarg