You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "clintropolis (via GitHub)" <gi...@apache.org> on 2024/01/06 04:57:33 UTC
[PR] fix bugs with expression virtual column indexes for expression virtual columns which refer to other virtual columns (druid)
clintropolis opened a new pull request, #15633:
URL: https://github.com/apache/druid/pull/15633
### 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:
* 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
#### Release note
<!-- Give your best effort to summarize your changes in a couple of sentences aimed toward Druid users.
If your change doesn't have end user impact, you can skip this section.
For tips about how to write a good release note, see [Release notes](https://github.com/apache/druid/blob/master/CONTRIBUTING.md#release-notes).
-->
<hr>
This PR has:
- [ ] been self-reviewed.
- [ ] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR doesn't have any relation to concurrency.)
- [ ] added documentation for new or modified features or behaviors.
- [ ] a release note entry in the PR description.
- [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [ ] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/dev/license.md)
- [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [ ] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [ ] added integration tests.
- [ ] been tested in a test Druid cluster.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] fix bugs with expression virtual column indexes for expression virtual columns which refer to other virtual columns (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #15633:
URL: https://github.com/apache/druid/pull/15633#discussion_r1444325325
##########
processing/src/main/java/org/apache/druid/segment/VirtualColumn.java:
##########
@@ -299,13 +300,16 @@
/**
* Get the {@link ColumnIndexSupplier} for the specified virtual column, with the assistance of a
- * {@link ColumnSelector} to allow reading things from segments. If the virtual column has no indexes, this method
- * will return null, or may also return a non-null supplier whose methods may return null values - having a supplier
- * is no guarantee that the column has indexes.
+ * {@link ColumnIndexSelector} to allow reading things from segments. If the virtual column has no indexes, this
+ * method will return null, or may also return a non-null supplier whose methods may return null values - having a
+ * supplier is no guarantee that the column has indexes.
*/
@SuppressWarnings("unused")
@Nullable
- default ColumnIndexSupplier getIndexSupplier(String columnName, ColumnSelector columnSelector)
+ default ColumnIndexSupplier getIndexSupplier(
+ String columnName,
Review 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...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] fix bugs with expression virtual column indexes for expression virtual columns which refer to other virtual columns (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on PR #15633:
URL: https://github.com/apache/druid/pull/15633#issuecomment-1880599061
code coverage failure is related to code that isn't run in default value mode so i think can be ignored
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] fix bugs with expression virtual column indexes for expression virtual columns which refer to other virtual columns (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis merged PR #15633:
URL: https://github.com/apache/druid/pull/15633
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] fix bugs with expression virtual column indexes for expression virtual columns which refer to other virtual columns (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on code in PR #15633:
URL: https://github.com/apache/druid/pull/15633#discussion_r1445332356
##########
processing/src/main/java/org/apache/druid/segment/VirtualColumn.java:
##########
@@ -299,13 +300,16 @@
/**
* Get the {@link ColumnIndexSupplier} for the specified virtual column, with the assistance of a
- * {@link ColumnSelector} to allow reading things from segments. If the virtual column has no indexes, this method
- * will return null, or may also return a non-null supplier whose methods may return null values - having a supplier
- * is no guarantee that the column has indexes.
+ * {@link ColumnIndexSelector} to allow reading things from segments. If the virtual column has no indexes, this
+ * method will return null, or may also return a non-null supplier whose methods may return null values - having a
+ * supplier is no guarantee that the column has indexes.
*/
@SuppressWarnings("unused")
@Nullable
- default ColumnIndexSupplier getIndexSupplier(String columnName, ColumnSelector columnSelector)
+ default ColumnIndexSupplier getIndexSupplier(
+ String columnName,
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] fix bugs with expression virtual column indexes for expression virtual columns which refer to other virtual columns (druid)
Posted by "github-advanced-security[bot] (via GitHub)" <gi...@apache.org>.
github-advanced-security[bot] commented on code in PR #15633:
URL: https://github.com/apache/druid/pull/15633#discussion_r1443648607
##########
processing/src/main/java/org/apache/druid/segment/VirtualColumn.java:
##########
@@ -299,13 +300,16 @@
/**
* Get the {@link ColumnIndexSupplier} for the specified virtual column, with the assistance of a
- * {@link ColumnSelector} to allow reading things from segments. If the virtual column has no indexes, this method
- * will return null, or may also return a non-null supplier whose methods may return null values - having a supplier
- * is no guarantee that the column has indexes.
+ * {@link ColumnIndexSelector} to allow reading things from segments. If the virtual column has no indexes, this
+ * method will return null, or may also return a non-null supplier whose methods may return null values - having a
+ * supplier is no guarantee that the column has indexes.
*/
@SuppressWarnings("unused")
@Nullable
- default ColumnIndexSupplier getIndexSupplier(String columnName, ColumnSelector columnSelector)
+ default ColumnIndexSupplier getIndexSupplier(
+ String columnName,
Review Comment:
## Useless parameter
The parameter 'columnName' is never used.
[Show more details](https://github.com/apache/druid/security/code-scanning/6422)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org