fix bugs with expression virtual column indexes for expression virtual columns which refer to other virtual columns#15633
Conversation
…l columns which refer to other virtual columns changes: * ColumnIndexSelector now extends ColumnSelector. The only real implementation of ColumnIndexSelector, ColumnSelectorColumnIndexSelector, already has a ColumnSelector, so this isn't very disruptive * removed getColumnNames from ColumnSelector since it was not used * VirtualColumns and VirtualColumn getIndexSupplier method now needs argument of ColumnIndexSelector instead of ColumnSelector, which allows expression virtual columns to correctly recognize other virtual columns, fixing an issue which would incorrectly handle other virtual columns as non-existent columns instead * fixed a bug with sql planner incorrectly not using expression filter for equality filters on columns with extractionFn and no virtual column registry
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @Nullable | ||
| default ColumnIndexSupplier getIndexSupplier(String columnName, ColumnSelector columnSelector) | ||
| default ColumnIndexSupplier getIndexSupplier( | ||
| String columnName, |
Check notice
Code scanning / CodeQL
Useless parameter
There was a problem hiding this comment.
I added columnName parameter to this method originally just in case stuff was using the 'usesDotNotation', unsure if the column name would be useful in such a situation, though perhaps it isn't really necessary and could be removed...
There was a problem hiding this comment.
There hasn't been much use that comes up for dot notation other than the original map virtual column. The JSON stuff could have used it, but we ended up going the more standard JSON_VALUE way instead. IMO it's fine to say simplify this and say that dot-notation virtual columns don't get to supply indexes. I'm OK either way though really.
|
code coverage failure is related to code that isn't run in default value mode so i think can be ignored |
gianm
left a comment
gianm
left a comment
There was a problem hiding this comment.
Approved, although if you want to remove columnName from getIndexSupplier, that's ok by me too.
4 participants
Description
Fixes some bugs with #15585 involving expression virtual columns which refer to other virtual columns, as well as an issue that can happen during SQL planning when extractionFn is specified for inputs to equality and null filters, but virtualColumnRegistry is null.
changes:
ColumnIndexSelectornow extendsColumnSelector. The only real implementation ofColumnIndexSelector,ColumnSelectorColumnIndexSelector, already has aColumnSelector, so this isn't very disruptivegetColumnNamesfromColumnSelectorsince it was not usedVirtualColumnsandVirtualColumnmethodgetIndexSuppliernow needs argument ofColumnIndexSelectorinstead ofColumnSelector, which allows expression virtual columns to correctly recognize other virtual columns, fixing an issue which would incorrectly handle other virtual columns as non-existent columns insteadRelease note
This PR has: