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 2023/01/19 01:15:55 UTC
[GitHub] [druid] imply-cheddar opened a new pull request, #13694: Adjust Operators to be Pausable
imply-cheddar opened a new pull request, #13694:
URL: https://github.com/apache/druid/pull/13694
This enables "merge" style operations that combine multiple streams.
This change includes a naive implementation of one such merge operator just to provide concrete evidence that the refactoring is effective.
The primary intent of the change can be seen by looking at the `Operator` interface. The change amounts to the
addition of a `Signal` enumeration that allows the Receiver to signal back to the Operator that it needs to pause
things. This then returns control to the caller to do something else (like call another Operator). The vast majority of
operators should never have to deal with this, but some that do merges will.
For a view into something that does a merge, this PR also includes a `SortedInnerJoinOperator` which implements a multi-way inner join across operators. The class is almost entirely business logic for conducting the join rather than interactions with the Operator interface, so it is quite large, but the intent was to prove that it is possible to create a
meaningful "merging" operator on top of the interface changes, which it achieves. I would recommend starting with
the `SortedInnerJoinOperatorTest` when reviewing just because that will show the intended usage of the operator.
Note, however, that this operator was created merely to exercise the interface change, as such it is not wired up into
planning no is there a query path that can exercise it yet. Now that I am certain the interface can handle all of the
needs of the data processing pipeline, I intend to go back to fleshing out the full test suite for window functions
and then can come back to this.
In terms of validation of the interface, there is likely one more validation to conduct, though I am fairly certain that it will be relatively simple and leave as a future exercise: the implementation of a `FrameProcessor` based on `Operator`.
This PR has:
- [x] been self-reviewed.
- [x] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [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.
--
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 #13694: Adjust Operators to be Pausable
Posted by "imply-cheddar (via GitHub)" <gi...@apache.org>.
imply-cheddar commented on code in PR #13694:
URL: https://github.com/apache/druid/pull/13694#discussion_r1084721382
##########
processing/src/main/java/org/apache/druid/query/rowsandcols/util/FindResult.java:
##########
@@ -0,0 +1,69 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.query.rowsandcols.util;
+
+public class FindResult
Review Comment:
"dealing in `int`" would actually mean `dealing in int[]` because I am often returning a start/end range. Once we are dealing in `int[]`, we have the overhead of a reference to an object anyway, so this seemed okay.
If we really wanted to fix this, we'd likely need to make the finder thingie itself stateful and have getters on that. That would probably be better, but can be an activity for a later 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 #13694: Adjust Operators to be Pausable
Posted by GitBox <gi...@apache.org>.
imply-cheddar commented on code in PR #13694:
URL: https://github.com/apache/druid/pull/13694#discussion_r1080766048
##########
processing/src/main/java/org/apache/druid/query/rowsandcols/RearrangedRowsAndColumns.java:
##########
@@ -72,13 +85,14 @@
@Override
public int numRows()
{
- return pointers.length;
+ return end - start;
Review Comment:
There's a validation that this is positive, so this check should be able to 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
[GitHub] [druid] imply-cheddar commented on a diff in pull request #13694: Adjust Operators to be Pausable
Posted by "imply-cheddar (via GitHub)" <gi...@apache.org>.
imply-cheddar commented on code in PR #13694:
URL: https://github.com/apache/druid/pull/13694#discussion_r1084719363
##########
processing/src/main/java/org/apache/druid/query/rowsandcols/column/DoubleArrayColumn.java:
##########
@@ -126,4 +74,122 @@ public <T> T as(Class<? extends T> clazz)
}
return null;
}
+
+ private class MyColumnAccessor implements BinarySearchableAccessor
Review Comment:
Generically speaking, yes it would be best to validate that first before returning the thing.
As a confounding factor, even if the whole column isn't sorted, it could be sorted in the context of another column. I.e. if a set of rows are sorted by `(col1, col2)`, then `col2` is not actually sorted if you look at the data, but when you access it in conjunction with `col1`, it is. I agree that we should do more to figure this sort of thing out and fail if it's violated, but, for now, the code is counting on the query planning to do things correctly
--
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] cheddar merged pull request #13694: Adjust Operators to be Pausable
Posted by "cheddar (via GitHub)" <gi...@apache.org>.
cheddar merged PR #13694:
URL: https://github.com/apache/druid/pull/13694
--
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] github-code-scanning[bot] commented on a diff in pull request #13694: Adjust Operators to be Pausable
Posted by GitBox <gi...@apache.org>.
github-code-scanning[bot] commented on code in PR #13694:
URL: https://github.com/apache/druid/pull/13694#discussion_r1080744790
##########
processing/src/main/java/org/apache/druid/query/operator/join/SortedInnerJoinOperator.java:
##########
@@ -0,0 +1,550 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.query.operator.join;
+
+import org.apache.druid.collections.fastutil.DruidIntList;
+import org.apache.druid.java.util.common.IAE;
+import org.apache.druid.java.util.common.ISE;
+import org.apache.druid.java.util.common.io.Closer;
+import org.apache.druid.java.util.common.logger.Logger;
+import org.apache.druid.query.operator.Operator;
+import org.apache.druid.query.rowsandcols.MapOfColumnsRowsAndColumns;
+import org.apache.druid.query.rowsandcols.RearrangedRowsAndColumns;
+import org.apache.druid.query.rowsandcols.RowsAndColumns;
+import org.apache.druid.query.rowsandcols.column.Column;
+import org.apache.druid.query.rowsandcols.semantic.SortedMatrixMaker;
+import org.apache.druid.query.rowsandcols.util.FindResult;
+
+import javax.annotation.Nullable;
+import java.io.Closeable;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.concurrent.atomic.AtomicBoolean;
+
+/**
+ * An operator that can join the data streams from other operators. Data must be provided in the same sorted
+ * order between the different operators.
+ * <p>
+ * Performs an InnerJoin based on the equality of two sets of fields. Null is considered a meaningful value
+ * when comparing the two streams. If null is intended to be excluded, it should be removed through a filter.
+ * <p>
+ * This class was created more as an exercise in ensuring that something meaningful can be made to do combinations
+ * of Operators (and ensure the interface is correct). There are tests that show that this class works, but those
+ * tests are not (yet) considered exhaustive. This paragraph in the comments should exist as a cautionary indication
+ * that if/when this class is dusted off for use again, there might be bugs yet lurking and it should likely start
+ * with fleshing out the tests.
+ */
+public class SortedInnerJoinOperator implements Operator
+{
+ private static final Logger log = new Logger(SortedInnerJoinOperator.class);
+
+ private final ArrayList<JoinPart> parts;
+ private final JoinConfig config;
+
+ public SortedInnerJoinOperator(
+ List<JoinPartDefn> partDefns,
+ JoinConfig config
+ )
+ {
+ this.parts = new ArrayList<>(partDefns.size());
+ for (JoinPartDefn partDefn : partDefns) {
+ parts.add(new JoinPart(partDefn.getOp(), partDefn.getJoinFields(), partDefn.getProjectFields()));
+ }
+
+ this.config = config;
+ }
+
+ @Override
+ public Closeable goOrContinue(Closeable continuation, Receiver receiver)
+ {
+ JoinLogic joinLogic;
+ if (continuation == null) {
+ joinLogic = new JoinLogic(config, parts);
+ } else {
+ joinLogic = (JoinLogic) continuation;
+ }
+
+ try {
+ joinLogic.go(receiver);
+ switch (joinLogic.state) {
+ case NEEDS_DATA:
+ case READY:
+ throw new ISE("joinLogic.go() exited with state[%s], should never happen.", joinLogic.state);
+ case COMPLETE:
+ return null;
+ case PAUSED:
+ return joinLogic;
+ default:
+ throw new ISE("Unknown state[%s]", joinLogic.state);
+ }
+ }
+ catch (RuntimeException e) {
+ try {
+ joinLogic.close();
+ }
+ catch (Exception ex) {
+ e.addSuppressed(ex);
+ }
+ throw e;
+ }
+ }
+
+ private static class JoinLogic implements Closeable
+ {
+ private final JoinConfig config;
+
+ private final ArrayList<JoinPart> joinParts;
+
+ private State state;
+ private int nextPositionToLoad;
+
+ private JoinLogic(
+ JoinConfig config,
+ ArrayList<JoinPart> joinParts
+ )
+ {
+ this.config = config;
+ this.joinParts = joinParts;
+
+ setNextPositionToLoad(joinParts.size() - 1);
+ }
+
+ public void go(Receiver receiver)
+ {
+ while (state != State.COMPLETE) {
+ final int position = nextPositionToLoad;
+ final JoinPart joinPart = joinParts.get(position);
+ //noinspection VariableNotUsedInsideIf
+ if (joinPart.curr != null) {
+ throw new ISE("loading data for position[%d], but it already had data!? Probably a bug!", position);
+ }
+
+ joinPart.goOrContinue(new Receiver()
+ {
+ @Override
+ public Signal push(RowsAndColumns rac)
+ {
+ joinPart.setCurr(rac);
+ process(receiver);
+
+ switch (state) {
+ case COMPLETE:
+ return Signal.STOP;
+ case NEEDS_DATA:
+ return Signal.GO;
+ case PAUSED:
+ return Signal.PAUSE;
+ case READY:
+ throw new ISE("Was in state READY after process returned!?");
+ default:
+ throw new ISE("Unknown state[%s]", state);
+ }
+ }
+
+ @Override
+ public void completed()
+ {
+ joinPart.complete.set(true);
+ }
+ });
+
+ if (joinPart.continuation == null && joinPart.needsData() && joinPart.isComplete()) {
+ // In this case, (1) the op returned null, (2) we don't have anything buffered to process and (3) completed
+ // was called. We are done.
+ state = State.COMPLETE;
+ }
+
+ switch (state) {
Review Comment:
## Missing enum case in switch
Switch statement does not have a case for [NEEDS_DATA](1).
[Show more details](https://github.com/apache/druid/security/code-scanning/3698)
##########
processing/src/main/java/org/apache/druid/query/rowsandcols/RearrangedRowsAndColumns.java:
##########
@@ -72,13 +85,14 @@
@Override
public int numRows()
{
- return pointers.length;
+ return end - start;
Review Comment:
## User-controlled data in arithmetic expression
This arithmetic expression depends on a [user-provided value](1), potentially causing an underflow.
This arithmetic expression depends on a [user-provided value](2), potentially causing an underflow.
This arithmetic expression depends on a [user-provided value](3), potentially causing an underflow.
This arithmetic expression depends on a [user-provided value](4), potentially causing an underflow.
This arithmetic expression depends on a [user-provided value](5), potentially causing an underflow.
[Show more details](https://github.com/apache/druid/security/code-scanning/3700)
##########
processing/src/main/java/org/apache/druid/query/operator/join/SortedInnerJoinOperator.java:
##########
@@ -0,0 +1,550 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.query.operator.join;
+
+import org.apache.druid.collections.fastutil.DruidIntList;
+import org.apache.druid.java.util.common.IAE;
+import org.apache.druid.java.util.common.ISE;
+import org.apache.druid.java.util.common.io.Closer;
+import org.apache.druid.java.util.common.logger.Logger;
+import org.apache.druid.query.operator.Operator;
+import org.apache.druid.query.rowsandcols.MapOfColumnsRowsAndColumns;
+import org.apache.druid.query.rowsandcols.RearrangedRowsAndColumns;
+import org.apache.druid.query.rowsandcols.RowsAndColumns;
+import org.apache.druid.query.rowsandcols.column.Column;
+import org.apache.druid.query.rowsandcols.semantic.SortedMatrixMaker;
+import org.apache.druid.query.rowsandcols.util.FindResult;
+
+import javax.annotation.Nullable;
+import java.io.Closeable;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.concurrent.atomic.AtomicBoolean;
+
+/**
+ * An operator that can join the data streams from other operators. Data must be provided in the same sorted
+ * order between the different operators.
+ * <p>
+ * Performs an InnerJoin based on the equality of two sets of fields. Null is considered a meaningful value
+ * when comparing the two streams. If null is intended to be excluded, it should be removed through a filter.
+ * <p>
+ * This class was created more as an exercise in ensuring that something meaningful can be made to do combinations
+ * of Operators (and ensure the interface is correct). There are tests that show that this class works, but those
+ * tests are not (yet) considered exhaustive. This paragraph in the comments should exist as a cautionary indication
+ * that if/when this class is dusted off for use again, there might be bugs yet lurking and it should likely start
+ * with fleshing out the tests.
+ */
+public class SortedInnerJoinOperator implements Operator
+{
+ private static final Logger log = new Logger(SortedInnerJoinOperator.class);
+
+ private final ArrayList<JoinPart> parts;
+ private final JoinConfig config;
+
+ public SortedInnerJoinOperator(
+ List<JoinPartDefn> partDefns,
+ JoinConfig config
+ )
+ {
+ this.parts = new ArrayList<>(partDefns.size());
+ for (JoinPartDefn partDefn : partDefns) {
+ parts.add(new JoinPart(partDefn.getOp(), partDefn.getJoinFields(), partDefn.getProjectFields()));
+ }
+
+ this.config = config;
+ }
+
+ @Override
+ public Closeable goOrContinue(Closeable continuation, Receiver receiver)
+ {
+ JoinLogic joinLogic;
+ if (continuation == null) {
+ joinLogic = new JoinLogic(config, parts);
+ } else {
+ joinLogic = (JoinLogic) continuation;
+ }
+
+ try {
+ joinLogic.go(receiver);
+ switch (joinLogic.state) {
+ case NEEDS_DATA:
+ case READY:
+ throw new ISE("joinLogic.go() exited with state[%s], should never happen.", joinLogic.state);
+ case COMPLETE:
+ return null;
+ case PAUSED:
+ return joinLogic;
+ default:
+ throw new ISE("Unknown state[%s]", joinLogic.state);
+ }
+ }
+ catch (RuntimeException e) {
+ try {
+ joinLogic.close();
+ }
+ catch (Exception ex) {
+ e.addSuppressed(ex);
+ }
+ throw e;
+ }
+ }
+
+ private static class JoinLogic implements Closeable
+ {
+ private final JoinConfig config;
+
+ private final ArrayList<JoinPart> joinParts;
+
+ private State state;
+ private int nextPositionToLoad;
+
+ private JoinLogic(
+ JoinConfig config,
+ ArrayList<JoinPart> joinParts
+ )
+ {
+ this.config = config;
+ this.joinParts = joinParts;
+
+ setNextPositionToLoad(joinParts.size() - 1);
+ }
+
+ public void go(Receiver receiver)
+ {
+ while (state != State.COMPLETE) {
+ final int position = nextPositionToLoad;
+ final JoinPart joinPart = joinParts.get(position);
+ //noinspection VariableNotUsedInsideIf
+ if (joinPart.curr != null) {
+ throw new ISE("loading data for position[%d], but it already had data!? Probably a bug!", position);
+ }
+
+ joinPart.goOrContinue(new Receiver()
+ {
+ @Override
+ public Signal push(RowsAndColumns rac)
+ {
+ joinPart.setCurr(rac);
+ process(receiver);
+
+ switch (state) {
+ case COMPLETE:
+ return Signal.STOP;
+ case NEEDS_DATA:
+ return Signal.GO;
+ case PAUSED:
+ return Signal.PAUSE;
+ case READY:
+ throw new ISE("Was in state READY after process returned!?");
+ default:
+ throw new ISE("Unknown state[%s]", state);
+ }
+ }
+
+ @Override
+ public void completed()
+ {
+ joinPart.complete.set(true);
+ }
+ });
+
+ if (joinPart.continuation == null && joinPart.needsData() && joinPart.isComplete()) {
+ // In this case, (1) the op returned null, (2) we don't have anything buffered to process and (3) completed
+ // was called. We are done.
+ state = State.COMPLETE;
+ }
+
+ switch (state) {
+ case READY:
+ throw new ISE("Don't expect READY state here, process() should've changed it to something else");
+ case PAUSED:
+ return;
+ case COMPLETE:
+ receiver.completed();
+
+ try {
+ close();
+ }
+ catch (IOException e) {
+ log.warn("Problem closing a join part[%d], ignoring because we are done anyway.", position);
+ }
+ break;
+ }
+ }
+ }
+
+ /**
+ * Processes whatever it can from the buffers, pushes the created RowsAndColumns to the receiver and
+ * returns which side we need more data from.
+ * <p>
+ * updates {@link #nextPositionToLoad} to a positive number if there is another position to load.
+ * sets it to -1 if processing is complete.
+ */
+ private void process(Receiver receiver)
+ {
+ // First check that we have something to work with for all parts of the join
+ for (int i = joinParts.size() - 1; i >= 0; --i) {
+ final JoinPart joinPart = joinParts.get(i);
+ if (joinPart.needsData()) {
+ if (joinPart.isComplete()) {
+ state = State.COMPLETE;
+ } else {
+ setNextPositionToLoad(i);
+ }
+ return;
+ }
+ }
+ state = State.READY;
+
+ DruidIntList[] rowsToInclude = new DruidIntList[joinParts.size()];
+ for (int i = 0; i < rowsToInclude.length; ++i) {
+ rowsToInclude[i] = new DruidIntList(config.getBufferSize());
+ if (joinParts.get(i).needsData()) {
+ throw new ISE("doJoin called while joinPart[%d] needed data. This is likely a bug", i);
+ }
+ }
+
+ final int finalIndex = joinParts.size() - 1;
+ final JoinPart finalPart = joinParts.get(finalIndex);
+ SortedMatrixMaker.SortedMatrix.MatrixRow row = null;
+ while (state == State.READY) {
+ if (row == null) {
+ row = finalPart.currMatrix.getRow(finalPart.currRowIndex);
+ }
+
+ row = joinRows(receiver, finalIndex, rowsToInclude, row);
+ }
+
+ pushRows(receiver, rowsToInclude);
+ }
+
+ private void pushRows(Receiver receiver, DruidIntList[] rowsToInclude)
+ {
+ int size = rowsToInclude[0].size();
+ if (size == 0) {
+ return;
+ }
+
+ LinkedHashMap<String, Column> cols = new LinkedHashMap<>();
+
+ for (int i = 0; i < joinParts.size(); ++i) {
+ final JoinPart part = joinParts.get(i);
+ RowsAndColumns remapped =
+ new RearrangedRowsAndColumns(rowsToInclude[i].elements(), 0, rowsToInclude[i].size(), part.curr);
+
+ for (String field : part.projectFields) {
+ cols.put(field, remapped.findColumn(field));
+ }
+ }
+
+ final Signal signal = receiver.push(new MapOfColumnsRowsAndColumns(cols, size));
+ switch (signal) {
Review Comment:
## Missing enum case in switch
Switch statement does not have a case for [GO](1).
[Show more details](https://github.com/apache/druid/security/code-scanning/3699)
--
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 #13694: Adjust Operators to be Pausable
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #13694:
URL: https://github.com/apache/druid/pull/13694#discussion_r1084682271
##########
processing/src/main/java/org/apache/druid/query/rowsandcols/column/DoubleArrayColumn.java:
##########
@@ -126,4 +74,122 @@ public <T> T as(Class<? extends T> clazz)
}
return null;
}
+
+ private class MyColumnAccessor implements BinarySearchableAccessor
Review Comment:
should this only be a binary searchable accessor if the column is sorted?
##########
processing/src/main/java/org/apache/druid/query/rowsandcols/util/FindResult.java:
##########
@@ -0,0 +1,69 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.query.rowsandcols.util;
+
+public class FindResult
Review Comment:
i know this makes stuff nicer, but wondering is this maybe expensive if we need to find a lot of things compared to just dealing in `int`?
Just thinking out loud, probably don't need to worry about it right now
##########
processing/src/main/java/org/apache/druid/query/rowsandcols/column/BinarySearchableAccessor.java:
##########
@@ -0,0 +1,54 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.query.rowsandcols.column;
+
+import org.apache.druid.java.util.common.ISE;
+import org.apache.druid.query.rowsandcols.util.FindResult;
+
+public interface BinarySearchableAccessor extends ColumnAccessor
+{
+ static BinarySearchableAccessor fromColumn(Column col)
+ {
+ final BinarySearchableAccessor retVal = col.as(BinarySearchableAccessor.class);
+
+ if (retVal == null) {
+ final ColumnAccessor accessor = col.toAccessor();
+ if (accessor instanceof BinarySearchableAccessor) {
+ // Why didn't they just return it from the as()!?!?! Who knows, ah well.
+ return (BinarySearchableAccessor) accessor;
+ }
+
+ throw new ISE("col[%s] is a no-go", col.getClass());
+ }
+ return retVal;
+ }
+
+ FindResult findNull(int startIndex, int endIndex);
+
+ FindResult findDouble(int startIndex, int endIndex, double val);
+
+ FindResult findFloat(int startIndex, int endIndex, float val);
+
+ FindResult findLong(int startIndex, int endIndex, long val);
+
+ FindResult findString(int startIndex, int endIndex, String val);
+
+ FindResult findComplex(int startIndex, int endIndex, Object val);
Review Comment:
should this be findObject instead of findComplex, that is if it should also handle other object types such as ARRAY<LONG>, ARRAY<STRING>, ARRAY<ARRAY<...>> etc? Or do you imagine array types will have some other method?
--
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 #13694: Adjust Operators to be Pausable
Posted by "imply-cheddar (via GitHub)" <gi...@apache.org>.
imply-cheddar commented on code in PR #13694:
URL: https://github.com/apache/druid/pull/13694#discussion_r1084720392
##########
processing/src/main/java/org/apache/druid/query/rowsandcols/column/BinarySearchableAccessor.java:
##########
@@ -0,0 +1,54 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.query.rowsandcols.column;
+
+import org.apache.druid.java.util.common.ISE;
+import org.apache.druid.query.rowsandcols.util.FindResult;
+
+public interface BinarySearchableAccessor extends ColumnAccessor
+{
+ static BinarySearchableAccessor fromColumn(Column col)
+ {
+ final BinarySearchableAccessor retVal = col.as(BinarySearchableAccessor.class);
+
+ if (retVal == null) {
+ final ColumnAccessor accessor = col.toAccessor();
+ if (accessor instanceof BinarySearchableAccessor) {
+ // Why didn't they just return it from the as()!?!?! Who knows, ah well.
+ return (BinarySearchableAccessor) accessor;
+ }
+
+ throw new ISE("col[%s] is a no-go", col.getClass());
+ }
+ return retVal;
+ }
+
+ FindResult findNull(int startIndex, int endIndex);
+
+ FindResult findDouble(int startIndex, int endIndex, double val);
+
+ FindResult findFloat(int startIndex, int endIndex, float val);
+
+ FindResult findLong(int startIndex, int endIndex, long val);
+
+ FindResult findString(int startIndex, int endIndex, String val);
+
+ FindResult findComplex(int startIndex, int endIndex, Object val);
Review Comment:
I imagine the Array types will have their own 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