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/10/21 18:52:35 UTC
[PR] snapshot column capabilities for realtime cursors (druid)
clintropolis opened a new pull request, #17386:
URL: https://github.com/apache/druid/pull/17386
### Description
changes:
* adds `CursorBuildSpec.getPhysicalColumns()` to allow specifying the set of required physical columns from a segment. if null, all columns are assumed to be required (e.g. full scan)
* `IncrementalIndexCursorFactory`/`IncrementalIndexCursorHolder` uses the physical columns from the cursor build spec to know which set of dimensions to 'snapshot' the capabilities for, allowing expression selectors on realtime queries to no longer be required to treat selectors from `StringDimensionIndexer` as multi-valued unless they truly are multi-valued. this fixes several bugs with expressions on realtime queries that change a value from `StringDimensionIndexer` to some type other than string, which would often result in a single element array from the column being handled as multi-valued
* `StringDimensionIndexer.setSparseIndexed()` now adds the default value to the dictionary when set
* `StringDimensionIndexer` column value selectors now always report that they are dictionary encoded, and that name lookup is possible in advance on their selectors (since set sparse adds the null value so the cardinality is correct)
* fixed a mistake that expression selectors for realtime queries with no null values could not use dictionary encoded selectors
#### Release note
_todo_
<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] snapshot column capabilities for realtime cursors (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #17386:
URL: https://github.com/apache/druid/pull/17386#discussion_r1866773101
##########
processing/src/main/java/org/apache/druid/segment/join/HashJoinSegmentCursorFactory.java:
##########
@@ -149,6 +166,27 @@ public CursorHolder makeCursorHolder(CursorBuildSpec spec)
);
cursorBuildSpecBuilder.setVirtualColumns(preJoinVirtualColumns);
+ // add all physical columns columns if they were originally set
+ if (physicalColumns != null) {
Review Comment:
this `CursorBuildSpec` is only for the base table so we only need to include all the stuff that is getting pushed down to the cursor we build on the base table, i'll update the comment
--
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] snapshot column capabilities for realtime cursors (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on code in PR #17386:
URL: https://github.com/apache/druid/pull/17386#discussion_r1873928291
##########
processing/src/main/java/org/apache/druid/segment/incremental/IncrementalIndexColumnSelectorFactory.java:
##########
@@ -91,7 +92,14 @@ class IncrementalIndexColumnSelectorFactory implements ColumnSelectorFactory, Ro
@Override
public ColumnCapabilities getColumnCapabilities(String column)
{
- return capabilitiesMap.get(column);
+ final ColumnCapabilities capabilities = capabilitiesMap.get(column);
+
+ DruidException.conditionalDefensive(
+ cursorBuildSpec.getPhysicalColumns() == null || capabilities != null || capabilitiesMap.containsKey(column),
+ "Asked for physical column capabilities for column[%s] which wasn't specified as required by the query, this is a bug",
Review Comment:
Not necessary to say "this is a bug"; it's implied by the category being "defensive".
##########
processing/src/main/java/org/apache/druid/segment/UnnestCursorFactory.java:
##########
@@ -97,6 +97,13 @@ public CursorHolder makeCursorHolder(CursorBuildSpec spec)
physicalColumns.add(column);
}
}
+ if (filter != null) {
+ for (String column : filter.getRequiredColumns()) {
+ if (spec.getVirtualColumns().getVirtualColumn(column) == null) {
Review Comment:
nit: could use `!spec.getVirtualColumns().exists(column)`
--
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] snapshot column capabilities for realtime cursors (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #17386:
URL: https://github.com/apache/druid/pull/17386#discussion_r1873779399
##########
processing/src/main/java/org/apache/druid/segment/incremental/IncrementalIndexColumnSelectorFactory.java:
##########
@@ -51,21 +54,44 @@ class IncrementalIndexColumnSelectorFactory implements ColumnSelectorFactory, Ro
IncrementalIndexColumnSelectorFactory(
IncrementalIndexRowSelector rowSelector,
IncrementalIndexRowHolder rowHolder,
- VirtualColumns virtualColumns,
+ CursorBuildSpec cursorBuildSpec,
Order timeOrder
)
{
this.rowSelector = rowSelector;
- this.virtualColumns = virtualColumns;
+ this.virtualColumns = cursorBuildSpec.getVirtualColumns();
this.timeOrder = timeOrder;
this.rowHolder = rowHolder;
+
+ final Map<String, ColumnCapabilities> capabilitiesMap = new HashMap<>();
+ if (cursorBuildSpec.getPhysicalColumns() == null) {
+ // add everything
+ for (String column : rowSelector.getDimensionNames(true)) {
+ capabilitiesMap.put(column, IncrementalIndexCursorFactory.snapshotColumnCapabilities(rowSelector, column));
+ }
+ for (String column : rowSelector.getMetricNames()) {
+ capabilitiesMap.put(column, IncrementalIndexCursorFactory.snapshotColumnCapabilities(rowSelector, column));
+ }
+ for (String column : cursorBuildSpec.getVirtualColumns().getColumnNames()) {
+ capabilitiesMap.put(column, IncrementalIndexCursorFactory.snapshotColumnCapabilities(rowSelector, column));
+ }
+ } else {
+ // just add required columns
+ for (String column : cursorBuildSpec.getPhysicalColumns()) {
+ capabilitiesMap.put(column, IncrementalIndexCursorFactory.snapshotColumnCapabilities(rowSelector, column));
+ }
+ // and virtual columns
+ for (String column : cursorBuildSpec.getVirtualColumns().getColumnNames()) {
+ capabilitiesMap.put(column, IncrementalIndexCursorFactory.snapshotColumnCapabilities(rowSelector, column));
+ }
+ }
this.snapshotColumnInspector = new ColumnInspector()
{
@Nullable
@Override
public ColumnCapabilities getColumnCapabilities(String column)
{
- return IncrementalIndexCursorFactory.snapshotColumnCapabilities(rowSelector, column);
+ return capabilitiesMap.get(column);
Review Comment:
i added validation for `getColumnCapabilities`. I think I would actually like to take it a step further and add this validation for all the selector factory methods for `IncrementalIndexColumnSelectorFactory`, `QueryableIndexColumnSelectorFactory` and `QueryableIndexVectorColumnSelectorFactory`, but have decided to do this as a follow-up since I feel like this PR already has enough stuff going on with it.
--
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] snapshot column capabilities for realtime cursors (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on code in PR #17386:
URL: https://github.com/apache/druid/pull/17386#discussion_r1866259222
##########
processing/src/main/java/org/apache/druid/segment/CursorBuildSpec.java:
##########
@@ -256,13 +275,11 @@ public CursorBuildSpecBuilder setInterval(Interval interval)
}
/**
- * @see CursorBuildSpec#getGroupingColumns()
+ * @see CursorBuildSpec#getPhysicalColumns()
*/
- public CursorBuildSpecBuilder setGroupingColumns(
- @Nullable List<String> groupingColumns
- )
+ public CursorBuildSpecBuilder setPhyiscalColumns(@Nullable Set<String> physicalColumns)
Review Comment:
Physical (spelling)
##########
processing/src/main/java/org/apache/druid/segment/join/HashJoinSegmentCursorFactory.java:
##########
@@ -149,6 +166,27 @@ public CursorHolder makeCursorHolder(CursorBuildSpec spec)
);
cursorBuildSpecBuilder.setVirtualColumns(preJoinVirtualColumns);
+ // add all physical columns columns if they were originally set
+ if (physicalColumns != null) {
Review Comment:
Do we need the condition columns from `clauses` too?
##########
processing/src/main/java/org/apache/druid/segment/incremental/IncrementalIndexColumnSelectorFactory.java:
##########
@@ -51,21 +54,44 @@ class IncrementalIndexColumnSelectorFactory implements ColumnSelectorFactory, Ro
IncrementalIndexColumnSelectorFactory(
IncrementalIndexRowSelector rowSelector,
IncrementalIndexRowHolder rowHolder,
- VirtualColumns virtualColumns,
+ CursorBuildSpec cursorBuildSpec,
Order timeOrder
)
{
this.rowSelector = rowSelector;
- this.virtualColumns = virtualColumns;
+ this.virtualColumns = cursorBuildSpec.getVirtualColumns();
this.timeOrder = timeOrder;
this.rowHolder = rowHolder;
+
+ final Map<String, ColumnCapabilities> capabilitiesMap = new HashMap<>();
+ if (cursorBuildSpec.getPhysicalColumns() == null) {
+ // add everything
+ for (String column : rowSelector.getDimensionNames(true)) {
+ capabilitiesMap.put(column, IncrementalIndexCursorFactory.snapshotColumnCapabilities(rowSelector, column));
+ }
+ for (String column : rowSelector.getMetricNames()) {
+ capabilitiesMap.put(column, IncrementalIndexCursorFactory.snapshotColumnCapabilities(rowSelector, column));
+ }
+ for (String column : cursorBuildSpec.getVirtualColumns().getColumnNames()) {
+ capabilitiesMap.put(column, IncrementalIndexCursorFactory.snapshotColumnCapabilities(rowSelector, column));
+ }
+ } else {
+ // just add required columns
+ for (String column : cursorBuildSpec.getPhysicalColumns()) {
+ capabilitiesMap.put(column, IncrementalIndexCursorFactory.snapshotColumnCapabilities(rowSelector, column));
+ }
+ // and virtual columns
+ for (String column : cursorBuildSpec.getVirtualColumns().getColumnNames()) {
+ capabilitiesMap.put(column, IncrementalIndexCursorFactory.snapshotColumnCapabilities(rowSelector, column));
+ }
+ }
this.snapshotColumnInspector = new ColumnInspector()
{
@Nullable
@Override
public ColumnCapabilities getColumnCapabilities(String column)
{
- return IncrementalIndexCursorFactory.snapshotColumnCapabilities(rowSelector, column);
+ return capabilitiesMap.get(column);
Review Comment:
I wonder if we should have a defensive check for calling `getColumnCapabilities` on a `column` that wasn't part of `physicalColumns`. At least one that is activated during tests, even if it's not activated during production.
--
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] snapshot column capabilities for realtime cursors (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #17386:
URL: https://github.com/apache/druid/pull/17386#discussion_r1873781637
##########
processing/src/main/java/org/apache/druid/segment/join/HashJoinSegmentCursorFactory.java:
##########
@@ -149,6 +166,27 @@ public CursorHolder makeCursorHolder(CursorBuildSpec spec)
);
cursorBuildSpecBuilder.setVirtualColumns(preJoinVirtualColumns);
+ // add all physical columns columns if they were originally set
+ if (physicalColumns != null) {
Review Comment:
this was a good point and missing, and the way things were also resulted in some join only columns getting pushed down to the base table cursor as physical columns. I've updated things to add all of the columns from the clauses and also filter out all of the columns with a join clause prefix to ensure that only base table columns are specified.
--
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] snapshot column capabilities for realtime cursors (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on code in PR #17386:
URL: https://github.com/apache/druid/pull/17386#discussion_r1869980626
##########
processing/src/main/java/org/apache/druid/segment/join/HashJoinSegmentCursorFactory.java:
##########
@@ -149,6 +166,27 @@ public CursorHolder makeCursorHolder(CursorBuildSpec spec)
);
cursorBuildSpecBuilder.setVirtualColumns(preJoinVirtualColumns);
+ // add all physical columns columns if they were originally set
+ if (physicalColumns != null) {
Review Comment:
I meant, if we have a condition like `x = j0.y`, doesn't the base table cursor need to have `x` added as a physical column? I would think yes but I don't see the code that adds it.
--
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] snapshot column capabilities for realtime cursors (druid)
Posted by "github-advanced-security[bot] (via GitHub)" <gi...@apache.org>.
github-advanced-security[bot] commented on code in PR #17386:
URL: https://github.com/apache/druid/pull/17386#discussion_r1872622110
##########
processing/src/main/java/org/apache/druid/segment/CursorBuildSpec.java:
##########
@@ -107,14 +113,13 @@
}
/**
- * Any columns which will be used for grouping by a query engine for the {@link CursorHolder}, useful for
- * specializing the {@link Cursor} or {@link org.apache.druid.segment.vector.VectorCursor} if any pre-aggregated
- * data is available.
+ * Set of physical columns required from a cursor. If null, and {@link #groupingColumns} is null or empty and
+ * {@link #aggregators} is null or empty, then a {@link CursorHolder} must assume that ALL columns are required
*/
@Nullable
- public List<String> getGroupingColumns()
+ public Set<String> getPhysicalColumns()
Review Comment:
## Exposing internal representation
getPhysicalColumns exposes the internal representation stored in field physicalColumns. The value may be modified [after this call to getPhysicalColumns](1).
[Show more details](https://github.com/apache/druid/security/code-scanning/8481)
--
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] snapshot column capabilities for realtime cursors (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm merged PR #17386:
URL: https://github.com/apache/druid/pull/17386
--
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