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 2023/02/09 03:28:19 UTC
[GitHub] [druid] clintropolis opened a new pull request, #13779: fix filtering nested field virtual column when used with non nested column input
clintropolis opened a new pull request, #13779:
URL: https://github.com/apache/druid/pull/13779
### Description
This PR fixes an issue when filtering on a `NestedFieldVirtualColumn` that is using an input column that is not a nested column, such as a standard string, long, float, or double column. #13732 updated `NestedFieldVirtualColumn` to be permissive and still able to create selectors when used with other column types, but I forgot to add this handling to the `getIndexSupplier` method, so attempting to filter these columns with a filter which would typically use an index, would result in an exception being thrown about the column being the wrong type, something like
```
Column [__time] is invalid type, found [class org.apache.druid.segment.column.LongsColumn] instead of [NestedDataComplexColumn]
```
This PR fixes this method to use the same logic as the selector methods, returning the underlying columns index supplier directly if selecting the 'root' path, else null as if the column didn't exist.
<hr>
This PR has:
- [x] been self-reviewed.
- [x] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [x] 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.
- [x] 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
[GitHub] [druid] imply-cheddar commented on a diff in pull request #13779: fix filtering nested field virtual column when used with non nested column input
Posted by "imply-cheddar (via GitHub)" <gi...@apache.org>.
imply-cheddar commented on code in PR #13779:
URL: https://github.com/apache/druid/pull/13779#discussion_r1100944253
##########
processing/src/main/java/org/apache/druid/segment/virtual/NestedFieldVirtualColumn.java:
##########
@@ -416,7 +417,7 @@ public VectorObjectSelector makeVectorObjectSelector(
this.columnName,
theColumn.makeVectorValueSelector(offset),
capabilities.toColumnType(),
- expectedType
+ expectedType != null ? expectedType : capabilities.toColumnType() // cast to itself in case the underlying column doesn't support object selector...
Review Comment:
The comment here isn't adding any value for me?
--
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
[GitHub] [druid] clintropolis commented on pull request #13779: fix filtering nested field virtual column when used with non nested column input
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on PR #13779:
URL: https://github.com/apache/druid/pull/13779#issuecomment-1424024562
i cancelled travis since it seems redundant
--
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
[GitHub] [druid] clintropolis commented on a diff in pull request #13779: fix filtering nested field virtual column when used with non nested column input
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #13779:
URL: https://github.com/apache/druid/pull/13779#discussion_r1100962754
##########
processing/src/main/java/org/apache/druid/segment/virtual/NestedFieldVirtualColumn.java:
##########
@@ -647,7 +650,10 @@ public ColumnCapabilities capabilities(ColumnInspector inspector, String columnN
final ColumnCapabilities capabilities = inspector.getColumnCapabilities(this.columnName);
if (capabilities != null) {
- if (!capabilities.isPrimitive() && capabilities.isDictionaryEncoded().isTrue()) {
+ // if the underlying column is a nested column (and persisted to disk, re: the dictionary encoded check)
Review Comment:
yeah, im certain there is a cooler way to do this, especially when the underlying inspector is the segment, but i'll save that for another day
--
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
[GitHub] [druid] imply-cheddar commented on a diff in pull request #13779: fix filtering nested field virtual column when used with non nested column input
Posted by "imply-cheddar (via GitHub)" <gi...@apache.org>.
imply-cheddar commented on code in PR #13779:
URL: https://github.com/apache/druid/pull/13779#discussion_r1100957121
##########
processing/src/main/java/org/apache/druid/segment/virtual/NestedFieldVirtualColumn.java:
##########
@@ -647,7 +650,10 @@ public ColumnCapabilities capabilities(ColumnInspector inspector, String columnN
final ColumnCapabilities capabilities = inspector.getColumnCapabilities(this.columnName);
if (capabilities != null) {
- if (!capabilities.isPrimitive() && capabilities.isDictionaryEncoded().isTrue()) {
+ // if the underlying column is a nested column (and persisted to disk, re: the dictionary encoded check)
Review Comment:
Do we really have no better way of determining if we have been persisted than checking `isDictionaryEncoded()`? Given that we are already a `NestedFieldVirtualColumn` and supposedly working with a `NestedField` that we understand the class of, I wonder if we couldn't have something more specific. Maybe for another day, but perhaps we could have a `NestedFieldColumnCapabilities` that we could cast to and get access to more specific methods?
--
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
[GitHub] [druid] clintropolis merged pull request #13779: fix filtering nested field virtual column when used with non nested column input
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis merged PR #13779:
URL: https://github.com/apache/druid/pull/13779
--
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