-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Upgrade calcite to 1.37.0 #16504
Upgrade calcite to 1.37.0 #16504
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like there are a few cases where a filtered agg has moved to be an aggregation on top of a virtual column. There's a risk that the change could end up as a performance regression, do we know why those plans have changed?
Aside from that, this seems to be all green and the bulk of the changes are in adding the parser.jj
rather than actual changes. So, I'm going ahead and pushing approve, but also think it's worth at least explaining why those SQL plans changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, it's not all green, not sure what I was thinking. So marking this as request changes just to make it all green. The current failure seems to be a test expectation mismatch (expected 2 got 5?).
The core set of things in this PR make sense to me though, so I think it's just getting to all green checks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there was a result mismatch when default values were used
.setVirtualColumns( | ||
expressionVirtualColumn( | ||
"v0", | ||
"case_searched((\"dim1\" != '1'),1,0)", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this CASE
statement was retained because the transformation to COUNT
may change behaviour when there are 0
input rows;
I've some ideas how to fix this - but didn't got to it yet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, it looks like it should turn green now, approving assuming it's all green.
CalciteSubqueryTest
AliasedOperatorConversion
was detectingCHAR_LENGTH
as not a function ; I've removed the checkkind
is passed for the createdSqlFunction
so I don't think this check is actually neededCalciteQueryTest#testExactCountDistinctWithFilter
is now executableI recommend to turn on whitespace ignore while taking a look at the changes