-
Notifications
You must be signed in to change notification settings - Fork 6.6k
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
Fix virtual columns having incorrect values after ORDER BY #54811
Conversation
This is an automated comment for commit 4958b16 with description of existing statuses. It's updated for the latest CI running ❌ Click here to open a full report in a separate page Successful checks
|
@@ -389,19 +389,19 @@ void addRequestedPathAndFileVirtualsToChunk( | |||
{ | |||
if (virtual_column.name == "_path") | |||
{ | |||
chunk.addColumn(virtual_column.type->createColumnConst(chunk.getNumRows(), path)); | |||
chunk.addColumn(virtual_column.type->createColumnConst(chunk.getNumRows(), path)->convertToFullColumnIfConst()); |
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.
If we would agree to proceed with option (1), then the comment is needed which describes this behaviour + we need to check other places where we inject virtual columns.
Originally virtual columns were materialized, but I changed it recently to ColumnConst to optimize queries like
With ColumnConst there are 2 optimizations:
We can compare master and build from this PR.
This PR:
Also we have such queries in performance tests, but for some reason not all queries from So, I would prefer trying to make ColumnConst work with ORDER BY if it's possible |
Maybe we can merge this PR as fast fix (and backport it to 23.8) and then think of better solution |
Trying to make ColumnConst work in #54866 |
@Avogar v23.8-must-backport ? |
Backport #54811 to 23.8: Fix virtual columns having incorrect values after ORDER BY
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Fixed virtual columns (e.g. _file) showing incorrect values with ORDER BY.
Repro:
The storage was outputting
ColumnConst
, then sorting transform was concatenating them usinginsertRangeFrom()
, which doesn't check if the values are equal.I don't know what's the intended way for this to work.
(1) Maybe storages are supposed to produce full columns only?
(2) Maybe
ColumnConst::insertRangeFrom()
should throw an exception if values are different (this won't fix this problem but would make it more noticeable).(3) Maybe there should be something like
static IColumn::concat(ColumnPtr & a, IColumn & b)
, which would automatically converta
to full column if values are different?This PR does the simplest thing (1) and just always materializes virtual columns. Lmk if that's not good enough.