You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2021/11/19 01:10:03 UTC
[GitHub] [druid] gianm opened a new pull request #11955: VirtualizedColumnSelectorFactory: Allow virtual columns to reference each other.
gianm opened a new pull request #11955:
URL: https://github.com/apache/druid/pull/11955
This matches the behavior of QueryableIndex and IncrementalIndex based cursors.
--
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] gianm commented on pull request #11955: VirtualizedColumnSelectorFactory: Allow virtual columns to reference each other.
Posted by GitBox <gi...@apache.org>.
gianm commented on pull request #11955:
URL: https://github.com/apache/druid/pull/11955#issuecomment-974685261
Ah, that's a good list. Yeah those should all use `this`, I think, to be consistent with the existing behavior of the selector-making methods. I'll change them and think a little about how we could improve the test coverage.
--
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] gianm commented on pull request #11955: VirtualizedColumnSelectorFactory: Allow virtual columns to reference each other.
Posted by GitBox <gi...@apache.org>.
gianm commented on pull request #11955:
URL: https://github.com/apache/druid/pull/11955#issuecomment-975209697
@clintropolis I think this should be enough to do it: https://github.com/apache/druid/pull/11955/commits/1b1eb1e6c195170d1c970c2a14e23472dcc05c22. With that change, the other call sites you mentioned look ok to 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] gianm commented on pull request #11955: VirtualizedColumnSelectorFactory: Allow virtual columns to reference each other.
Posted by GitBox <gi...@apache.org>.
gianm commented on pull request #11955:
URL: https://github.com/apache/druid/pull/11955#issuecomment-974565750
@clintropolis with regard to whether virtual column implementations would handle this properly, I want to stress that virtual columns referencing other virtual columns is _already possible_ for cursors backed by regular segments. They do their own virtual column handling and it already works this way. The VirtualizedColumnSelectorFactory is updated in this patch to match that behavior.
--
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] gianm edited a comment on pull request #11955: VirtualizedColumnSelectorFactory: Allow virtual columns to reference each other.
Posted by GitBox <gi...@apache.org>.
gianm edited a comment on pull request #11955:
URL: https://github.com/apache/druid/pull/11955#issuecomment-974565750
--
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] gianm commented on pull request #11955: VirtualizedColumnSelectorFactory: Allow virtual columns to reference each other.
Posted by GitBox <gi...@apache.org>.
gianm commented on pull request #11955:
URL: https://github.com/apache/druid/pull/11955#issuecomment-974685261
Ah, that's a good list. Yeah those should all use `this`, I think, to be consistent with the existing behavior of the selector-making methods. I'll change them and think a little about how we could improve the test coverage.
--
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] gianm edited a comment on pull request #11955: VirtualizedColumnSelectorFactory: Allow virtual columns to reference each other.
Posted by GitBox <gi...@apache.org>.
gianm edited a comment on pull request #11955:
URL: https://github.com/apache/druid/pull/11955#issuecomment-974565750
@clintropolis with regard to whether virtual column implementations would handle this properly, I want to stress that virtual columns referencing other virtual columns is _already possible_ for cursors backed by regular segments. They do their own virtual column handling and it already works this way. The VirtualizedColumnSelectorFactory is updated in this patch to match that behavior. It's used by RowBasedSegments and by PostJoinCursor.
--
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] gianm merged pull request #11955: Harmonize behavior when virtual columns reference each other.
Posted by GitBox <gi...@apache.org>.
gianm merged pull request #11955:
URL: https://github.com/apache/druid/pull/11955
--
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 #11955: VirtualizedColumnSelectorFactory: Allow virtual columns to reference each other.
Posted by GitBox <gi...@apache.org>.
clintropolis commented on pull request #11955:
URL: https://github.com/apache/druid/pull/11955#issuecomment-974495671
This seems like it should work for `ExpressionVirtualColumns` without issue, but we might need to add some additional guards to some other virtual columns which expect to be built on top of a physical column to do things with dictionaries and indexes, such as `ListFilteredVirtualColumn`, looking into some code to see if this should be done now or can be done later.
I'm also wondering if there is work SQL side to be done here to allow this, e.g. with `VirtualColumnRegistry` and how some of the expressions are being constructed, to allow SQL to plan into things like this. I think we would want some smarts to it so that it only breaks up an expression virtual column into multiple parts if there is some sub-part shared by all callers, but could also potentially be nice in the cases that have "native" optimized virtual column implementations, such as allowing expressions to call `ListFilteredVirtualColumn`.
--
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 #11955: VirtualizedColumnSelectorFactory: Allow virtual columns to reference each other.
Posted by GitBox <gi...@apache.org>.
clintropolis commented on pull request #11955:
URL: https://github.com/apache/druid/pull/11955#issuecomment-974574311
>@clintropolis with regard to whether virtual column implementations would handle this properly, I want to stress that virtual columns referencing other virtual columns is already possible for cursors backed by regular segments or by IncrementalIndexes. They do their own virtual column handling and they both already work this way. The VirtualizedColumnSelectorFactory is updated in this patch to match that behavior. It's used by RowBasedSegments and by PostJoinCursor.
Hmm, in that case there might be a couple of other things worth fixing so stuff works good.
these should probably use `this` instead of the inspector of the underlying index:
https://github.com/apache/druid/blob/master/processing/src/main/java/org/apache/druid/segment/QueryableIndexColumnSelectorFactory.java#L196
https://github.com/apache/druid/blob/master/processing/src/main/java/org/apache/druid/segment/vector/QueryableIndexVectorColumnSelectorFactory.java#L270
https://github.com/apache/druid/blob/master/processing/src/main/java/org/apache/druid/segment/virtual/VirtualizedColumnInspector.java#L55
I've had it in my head that this is broken for some reason, but admit the last time i really dug into it was a couple of years ago, so trying to remember things.
--
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