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 2022/12/20 05:22:45 UTC
[GitHub] [druid] imply-cheddar opened a new pull request, #13600: Switch operators to a push-style API
imply-cheddar opened a new pull request, #13600:
URL: https://github.com/apache/druid/pull/13600
Change the Operator interface introduced for Window functions to be more of a push-model. The impetus for this is to generate better stack traces. With the pull-based model, the stack trace of an exception shows the operator chain that still has work to do without any indication of which operators have done work so far.
With the change in the API introduced here, we will produce stack traces that start with the full operator chain running down the .go() method. Then the stack will walk back up towards the Receiver that was passed into the .go() method. This should produce a stack trace that tells us both the full operator chain that is being executed as well as give an indication of what actions have happened so far as the data has been processed.
It remains important that Operators are implemented directly without any usage of fluent-style programming. This is something that we will likely have to enforce through review.
This PR is a refactoring with no additional functionality.
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
[GitHub] [druid] github-code-scanning[bot] commented on a diff in pull request #13600: Switch operators to a push-style API
Posted by GitBox <gi...@apache.org>.
github-code-scanning[bot] commented on code in PR #13600:
URL: https://github.com/apache/druid/pull/13600#discussion_r1055072558
##########
processing/src/test/java/org/apache/druid/query/operator/OperatorTestHelper.java:
##########
@@ -0,0 +1,141 @@
+/*
+ * 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;
+
+import org.apache.druid.java.util.common.ISE;
+import org.apache.druid.query.operator.window.RowsAndColumnsHelper;
+import org.apache.druid.query.rowsandcols.RowsAndColumns;
+import org.junit.Assert;
+
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicLong;
+import java.util.function.Consumer;
+
+public class OperatorTestHelper
+{
+ private final Operator op;
+ private TestReceiver receiver;
+ private Consumer<TestReceiver> finalValidation;
+
+ public OperatorTestHelper(
+ Operator op
+ )
+ {
+ this.op = op;
+ }
+
+ public OperatorTestHelper expectRowsAndColumns(RowsAndColumnsHelper... helpers)
+ {
+ return withPushFn(
+ new JustPushMe()
+ {
+ int index = 0;
+
+ @Override
+ public boolean push(RowsAndColumns rac)
+ {
+ helpers[index++].validate(rac);
+ return true;
+ }
+ }
+ ).withFinalValidation(
+ testReceiver -> Assert.assertEquals(helpers.length, receiver.getNumPushed())
+ );
+ }
+
+ public OperatorTestHelper withReceiver(TestReceiver receiver)
+ {
+ if (this.receiver != null) {
+ throw new ISE("Receiver[%s] already set, cannot set it again[%s].", this.receiver, receiver);
Review Comment:
## Use of default toString()
Default toString(): TestReceiver inherits toString() from Object, and so is not suitable for printing.
[Show more details](https://github.com/apache/druid/security/code-scanning/3573)
##########
processing/src/test/java/org/apache/druid/query/operator/OperatorTestHelper.java:
##########
@@ -0,0 +1,141 @@
+/*
+ * 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;
+
+import org.apache.druid.java.util.common.ISE;
+import org.apache.druid.query.operator.window.RowsAndColumnsHelper;
+import org.apache.druid.query.rowsandcols.RowsAndColumns;
+import org.junit.Assert;
+
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicLong;
+import java.util.function.Consumer;
+
+public class OperatorTestHelper
+{
+ private final Operator op;
+ private TestReceiver receiver;
+ private Consumer<TestReceiver> finalValidation;
+
+ public OperatorTestHelper(
+ Operator op
+ )
+ {
+ this.op = op;
+ }
+
+ public OperatorTestHelper expectRowsAndColumns(RowsAndColumnsHelper... helpers)
+ {
+ return withPushFn(
+ new JustPushMe()
+ {
+ int index = 0;
+
+ @Override
+ public boolean push(RowsAndColumns rac)
+ {
+ helpers[index++].validate(rac);
+ return true;
+ }
+ }
+ ).withFinalValidation(
+ testReceiver -> Assert.assertEquals(helpers.length, receiver.getNumPushed())
+ );
+ }
+
+ public OperatorTestHelper withReceiver(TestReceiver receiver)
+ {
+ if (this.receiver != null) {
+ throw new ISE("Receiver[%s] already set, cannot set it again[%s].", this.receiver, receiver);
Review Comment:
## Use of default toString()
Default toString(): TestReceiver inherits toString() from Object, and so is not suitable for printing.
[Show more details](https://github.com/apache/druid/security/code-scanning/3574)
--
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 commented on pull request #13600: Switch operators to a push-style API
Posted by GitBox <gi...@apache.org>.
cheddar commented on PR #13600:
URL: https://github.com/apache/druid/pull/13600#issuecomment-1363645377
Most of the commentary focuses on javadoc and I don't want to wait for another round of CI on this PR just to update javadoc, so I'm going to merge and update the javadoc in a subsequent PR.
--
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 #13600: Switch operators to a push-style API
Posted by GitBox <gi...@apache.org>.
cheddar merged PR #13600:
URL: https://github.com/apache/druid/pull/13600
--
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 #13600: Switch operators to a push-style API
Posted by GitBox <gi...@apache.org>.
imply-cheddar commented on code in PR #13600:
URL: https://github.com/apache/druid/pull/13600#discussion_r1056061085
##########
processing/src/main/java/org/apache/druid/query/operator/Operator.java:
##########
@@ -22,72 +22,48 @@
import org.apache.druid.query.rowsandcols.RowsAndColumns;
/**
- * An Operator interface that intends to align closely with the Operators that other databases would also tend
- * to be implemented using.
+ * An Operator interface that intends to have implementations that align relatively closely with the Operators that
+ * other databases would also tend to be implemented using. While a lot of Operator interfaces tend to use a
+ * pull-based orientation, we use a push-based interface. This is to give us good stacktraces. Because of the
+ * organization of the go() method, the stack traces thrown out of an Operator will be
+ * 1. All of the go() calls from the top-level Operator down to the leaf Operator, this part of the stacktrace gives
+ * visibility into what all of the actions that we expect to happen from the operator chain are
+ * 2. All of the push() calls up until the exception happens, this part of the stack trace gives us a view of all
+ * of the things that have happened to the data up until the exception was thrown.
* <p>
- * The lifecycle of an operator is that, after creation, it should be opened, and then iterated using hasNext() and
- * next(). Finally, when the Operator is no longer useful, it should be closed.
+ * This "hour glass" structure of the stacktrace is by design. It is very important that implementations of this
+ * interface never resort to a fluent style, inheritance or other code structuring that removes the name of the active
+ * Operator from the stacktrace. It should always be possible to find ways to avoid code duplication and still keep
+ * the Operator's name on the stacktrace.
* <p>
- * Operator's methods mimic the methods of an {@code Iterator}, but it does not implement {@code Iterator}
- * intentionally. An operator should never be wrapped in an {@code Iterator}. Any code that does that should be
- * considered a bug and fixed. This is for two reasons:
- * <p>
- * 1. An Operator should never be passed around as an {@code Iterator}. An Operator must be closed, if an operator
- * gets returned as an {@code Iterator}, the code that sees the {@code Iterator} loses the knowledge that it's
- * dealing with an Operator and might not close it. Even something like a {@code CloseableIterator} is an
- * anti-pattern as it's possible to use it in a functional manner with code that loses track of the fact that it
- * must be closed.
- * 2. To avoid "fluent" style composition of functions on Operators. It is important that there never be a set of
- * functional primitives for things like map/filter/reduce to "simplify" the implementation of Operators. This is
- * because such fluency produces really hard to decipher stack traces as the stacktrace ends up being just a bunch
- * of calls from the scaffolding (map/filter/reduce) and not from the actual Operator itself. By not implementing
- * {@code Iterator} we are actively increasing the burden of trying to add such functional operations to the point
- * that hopefully, though code review, we can ensure that we never develop them. It is infinitely better to preserve
- * the stacktrace and "duplicate" the map/filter/reduce scaffolding code.
+ * The other benefit of the go() method is that it fully encapsulates the lifecycle of the underlying resources.
+ * This means that it should be possible to use try/finally blocks around calls to go() in order to ensure that
+ * resources are properly closed.
*/
public interface Operator
{
/**
- * Called to initiate the lifecycle of the Operator. If an operator needs to checkout resources or anything to do
- * its work, this is probably the place to do it.
+ * Tells the Operation to start doing its work. Data will be pushed into the Receiver.
*
- * Work should *never* be done in this method, this method only exists to acquire resources that are known to be
- * needed before doing any work. As a litmus test, if there is ever a call to `op.next()` inside of this method,
- * then something has been done wrong as that call to `.next()` is actually doing work. Such code should be moved
- * into being lazily evaluated as part of a call to `.next()`.
+ * @param receiver a receiver that will receive data
*/
- void open();
+ void go(Receiver receiver);
- /**
- * Returns the next RowsAndColumns object that the Operator can produce. Behavior is undefined if
- * {@link #hasNext} returns false.
- *
- * @return the next RowsAndColumns object that the operator can produce
- */
- RowsAndColumns next();
+ interface Receiver
+ {
+ /**
+ * Used to push data. Return value indicates if more data will be accepted. If false, push should not
+ * be called anymore.
+ *
+ * @param rac {@link RowsAndColumns} of data
+ * @return a boolean value indicating if more data will be accepted. If false, push should never be called
+ * anymore
+ */
+ boolean push(RowsAndColumns rac);
- /**
- * Used to identify if it is safe to call {@link #next}
- *
- * @return true if it is safe to call {@link #next}
- */
- boolean hasNext();
-
- /**
- * Closes this Operator. The cascade flag can be used to identify that the intent is to close this operator
- * and only this operator without actually closing child operators. Other databases us this sort of functionality
- * with a planner that is watching over all of the objects and force-closes even if they were closed during normal
- * operations. In Druid, in the data pipeline where this was introduced, we are guaranteed to always have close
- * called regardless of errors or exceptions during processing, as such, at time of introduction, there is no
- * call that passes false for cascade.
- * <p>
- * That said, given that this is a common thing for these interfaces for other databases, we want to preserve the
- * optionality of being able to leverage what they do. As such, we define the method this way with the belief
- * that it might be used in the future. Semantically, this means that all implementations of Operators must
- * expect to be closed multiple times. I.e. after being closed, it is an error for open, next or hasNext to be
- * called, but close can be called any number of times.
- *
- * @param cascade whether to call close on child operators.
- */
- void close(boolean cascade);
+ /**
+ * Used to indicate that no more data will ever come
+ */
+ void completed();
Review Comment:
Good question, this is definitely not `close()` with a different name and I should call that out better in the javadoc. It is more correct to say that this is `flush()` with a different name, except it's expected that after `flush()` no more `write()`s would happen. It's probably going to be tempting to try to close resources in here, but there is no guarantee that `complete()` will be called if an exception destroys the operations during a `push()` call.
--
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 #13600: Switch operators to a push-style API
Posted by GitBox <gi...@apache.org>.
imply-cheddar commented on code in PR #13600:
URL: https://github.com/apache/druid/pull/13600#discussion_r1056061598
##########
processing/src/main/java/org/apache/druid/query/operator/Operator.java:
##########
@@ -22,72 +22,48 @@
import org.apache.druid.query.rowsandcols.RowsAndColumns;
/**
- * An Operator interface that intends to align closely with the Operators that other databases would also tend
- * to be implemented using.
+ * An Operator interface that intends to have implementations that align relatively closely with the Operators that
+ * other databases would also tend to be implemented using. While a lot of Operator interfaces tend to use a
+ * pull-based orientation, we use a push-based interface. This is to give us good stacktraces. Because of the
+ * organization of the go() method, the stack traces thrown out of an Operator will be
+ * 1. All of the go() calls from the top-level Operator down to the leaf Operator, this part of the stacktrace gives
+ * visibility into what all of the actions that we expect to happen from the operator chain are
+ * 2. All of the push() calls up until the exception happens, this part of the stack trace gives us a view of all
+ * of the things that have happened to the data up until the exception was thrown.
* <p>
- * The lifecycle of an operator is that, after creation, it should be opened, and then iterated using hasNext() and
- * next(). Finally, when the Operator is no longer useful, it should be closed.
+ * This "hour glass" structure of the stacktrace is by design. It is very important that implementations of this
+ * interface never resort to a fluent style, inheritance or other code structuring that removes the name of the active
+ * Operator from the stacktrace. It should always be possible to find ways to avoid code duplication and still keep
+ * the Operator's name on the stacktrace.
* <p>
- * Operator's methods mimic the methods of an {@code Iterator}, but it does not implement {@code Iterator}
- * intentionally. An operator should never be wrapped in an {@code Iterator}. Any code that does that should be
- * considered a bug and fixed. This is for two reasons:
- * <p>
- * 1. An Operator should never be passed around as an {@code Iterator}. An Operator must be closed, if an operator
- * gets returned as an {@code Iterator}, the code that sees the {@code Iterator} loses the knowledge that it's
- * dealing with an Operator and might not close it. Even something like a {@code CloseableIterator} is an
- * anti-pattern as it's possible to use it in a functional manner with code that loses track of the fact that it
- * must be closed.
- * 2. To avoid "fluent" style composition of functions on Operators. It is important that there never be a set of
- * functional primitives for things like map/filter/reduce to "simplify" the implementation of Operators. This is
- * because such fluency produces really hard to decipher stack traces as the stacktrace ends up being just a bunch
- * of calls from the scaffolding (map/filter/reduce) and not from the actual Operator itself. By not implementing
- * {@code Iterator} we are actively increasing the burden of trying to add such functional operations to the point
- * that hopefully, though code review, we can ensure that we never develop them. It is infinitely better to preserve
- * the stacktrace and "duplicate" the map/filter/reduce scaffolding code.
+ * The other benefit of the go() method is that it fully encapsulates the lifecycle of the underlying resources.
+ * This means that it should be possible to use try/finally blocks around calls to go() in order to ensure that
+ * resources are properly closed.
*/
public interface Operator
{
/**
- * Called to initiate the lifecycle of the Operator. If an operator needs to checkout resources or anything to do
- * its work, this is probably the place to do it.
+ * Tells the Operation to start doing its work. Data will be pushed into the Receiver.
*
- * Work should *never* be done in this method, this method only exists to acquire resources that are known to be
- * needed before doing any work. As a litmus test, if there is ever a call to `op.next()` inside of this method,
- * then something has been done wrong as that call to `.next()` is actually doing work. Such code should be moved
- * into being lazily evaluated as part of a call to `.next()`.
+ * @param receiver a receiver that will receive data
*/
- void open();
+ void go(Receiver receiver);
- /**
- * Returns the next RowsAndColumns object that the Operator can produce. Behavior is undefined if
- * {@link #hasNext} returns false.
- *
- * @return the next RowsAndColumns object that the operator can produce
- */
- RowsAndColumns next();
+ interface Receiver
+ {
+ /**
+ * Used to push data. Return value indicates if more data will be accepted. If false, push should not
+ * be called anymore.
+ *
+ * @param rac {@link RowsAndColumns} of data
+ * @return a boolean value indicating if more data will be accepted. If false, push should never be called
+ * anymore
+ */
+ boolean push(RowsAndColumns rac);
Review Comment:
undefined
--
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 #13600: Switch operators to a push-style API
Posted by GitBox <gi...@apache.org>.
imply-cheddar commented on code in PR #13600:
URL: https://github.com/apache/druid/pull/13600#discussion_r1056061426
##########
processing/src/main/java/org/apache/druid/query/operator/SegmentToRowsAndColumnsOperator.java:
##########
@@ -36,33 +39,22 @@ public SegmentToRowsAndColumnsOperator(
}
@Override
- public void open()
+ public void go(Receiver receiver)
{
-
- }
-
- @Override
- public RowsAndColumns next()
- {
- hasNext = false;
-
- RowsAndColumns rac = segment.as(RowsAndColumns.class);
- if (rac != null) {
- return rac;
+ try (final CloseableShapeshifter shifty = segment.as(CloseableShapeshifter.class)) {
Review Comment:
I agree that it's a dizzying mess in `Segment`. Hopefully, this contributes to eventually making it a less-dizzying mess in some future world.
--
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 #13600: Switch operators to a push-style API
Posted by GitBox <gi...@apache.org>.
clintropolis commented on code in PR #13600:
URL: https://github.com/apache/druid/pull/13600#discussion_r1056046648
##########
processing/src/main/java/org/apache/druid/query/operator/Operator.java:
##########
@@ -22,72 +22,48 @@
import org.apache.druid.query.rowsandcols.RowsAndColumns;
/**
- * An Operator interface that intends to align closely with the Operators that other databases would also tend
- * to be implemented using.
+ * An Operator interface that intends to have implementations that align relatively closely with the Operators that
+ * other databases would also tend to be implemented using. While a lot of Operator interfaces tend to use a
+ * pull-based orientation, we use a push-based interface. This is to give us good stacktraces. Because of the
+ * organization of the go() method, the stack traces thrown out of an Operator will be
+ * 1. All of the go() calls from the top-level Operator down to the leaf Operator, this part of the stacktrace gives
+ * visibility into what all of the actions that we expect to happen from the operator chain are
+ * 2. All of the push() calls up until the exception happens, this part of the stack trace gives us a view of all
+ * of the things that have happened to the data up until the exception was thrown.
* <p>
- * The lifecycle of an operator is that, after creation, it should be opened, and then iterated using hasNext() and
- * next(). Finally, when the Operator is no longer useful, it should be closed.
+ * This "hour glass" structure of the stacktrace is by design. It is very important that implementations of this
+ * interface never resort to a fluent style, inheritance or other code structuring that removes the name of the active
+ * Operator from the stacktrace. It should always be possible to find ways to avoid code duplication and still keep
+ * the Operator's name on the stacktrace.
* <p>
- * Operator's methods mimic the methods of an {@code Iterator}, but it does not implement {@code Iterator}
- * intentionally. An operator should never be wrapped in an {@code Iterator}. Any code that does that should be
- * considered a bug and fixed. This is for two reasons:
- * <p>
- * 1. An Operator should never be passed around as an {@code Iterator}. An Operator must be closed, if an operator
- * gets returned as an {@code Iterator}, the code that sees the {@code Iterator} loses the knowledge that it's
- * dealing with an Operator and might not close it. Even something like a {@code CloseableIterator} is an
- * anti-pattern as it's possible to use it in a functional manner with code that loses track of the fact that it
- * must be closed.
- * 2. To avoid "fluent" style composition of functions on Operators. It is important that there never be a set of
- * functional primitives for things like map/filter/reduce to "simplify" the implementation of Operators. This is
- * because such fluency produces really hard to decipher stack traces as the stacktrace ends up being just a bunch
- * of calls from the scaffolding (map/filter/reduce) and not from the actual Operator itself. By not implementing
- * {@code Iterator} we are actively increasing the burden of trying to add such functional operations to the point
- * that hopefully, though code review, we can ensure that we never develop them. It is infinitely better to preserve
- * the stacktrace and "duplicate" the map/filter/reduce scaffolding code.
+ * The other benefit of the go() method is that it fully encapsulates the lifecycle of the underlying resources.
+ * This means that it should be possible to use try/finally blocks around calls to go() in order to ensure that
+ * resources are properly closed.
*/
public interface Operator
{
/**
- * Called to initiate the lifecycle of the Operator. If an operator needs to checkout resources or anything to do
- * its work, this is probably the place to do it.
+ * Tells the Operation to start doing its work. Data will be pushed into the Receiver.
*
- * Work should *never* be done in this method, this method only exists to acquire resources that are known to be
- * needed before doing any work. As a litmus test, if there is ever a call to `op.next()` inside of this method,
- * then something has been done wrong as that call to `.next()` is actually doing work. Such code should be moved
- * into being lazily evaluated as part of a call to `.next()`.
+ * @param receiver a receiver that will receive data
*/
- void open();
+ void go(Receiver receiver);
- /**
- * Returns the next RowsAndColumns object that the Operator can produce. Behavior is undefined if
- * {@link #hasNext} returns false.
- *
- * @return the next RowsAndColumns object that the operator can produce
- */
- RowsAndColumns next();
+ interface Receiver
+ {
+ /**
+ * Used to push data. Return value indicates if more data will be accepted. If false, push should not
+ * be called anymore.
+ *
+ * @param rac {@link RowsAndColumns} of data
+ * @return a boolean value indicating if more data will be accepted. If false, push should never be called
+ * anymore
+ */
+ boolean push(RowsAndColumns rac);
Review Comment:
what happens if `push` is called again?
##########
processing/src/main/java/org/apache/druid/query/operator/Operator.java:
##########
@@ -22,72 +22,48 @@
import org.apache.druid.query.rowsandcols.RowsAndColumns;
/**
- * An Operator interface that intends to align closely with the Operators that other databases would also tend
- * to be implemented using.
+ * An Operator interface that intends to have implementations that align relatively closely with the Operators that
+ * other databases would also tend to be implemented using. While a lot of Operator interfaces tend to use a
+ * pull-based orientation, we use a push-based interface. This is to give us good stacktraces. Because of the
+ * organization of the go() method, the stack traces thrown out of an Operator will be
+ * 1. All of the go() calls from the top-level Operator down to the leaf Operator, this part of the stacktrace gives
+ * visibility into what all of the actions that we expect to happen from the operator chain are
+ * 2. All of the push() calls up until the exception happens, this part of the stack trace gives us a view of all
+ * of the things that have happened to the data up until the exception was thrown.
* <p>
- * The lifecycle of an operator is that, after creation, it should be opened, and then iterated using hasNext() and
- * next(). Finally, when the Operator is no longer useful, it should be closed.
+ * This "hour glass" structure of the stacktrace is by design. It is very important that implementations of this
+ * interface never resort to a fluent style, inheritance or other code structuring that removes the name of the active
+ * Operator from the stacktrace. It should always be possible to find ways to avoid code duplication and still keep
+ * the Operator's name on the stacktrace.
* <p>
- * Operator's methods mimic the methods of an {@code Iterator}, but it does not implement {@code Iterator}
- * intentionally. An operator should never be wrapped in an {@code Iterator}. Any code that does that should be
- * considered a bug and fixed. This is for two reasons:
- * <p>
- * 1. An Operator should never be passed around as an {@code Iterator}. An Operator must be closed, if an operator
- * gets returned as an {@code Iterator}, the code that sees the {@code Iterator} loses the knowledge that it's
- * dealing with an Operator and might not close it. Even something like a {@code CloseableIterator} is an
- * anti-pattern as it's possible to use it in a functional manner with code that loses track of the fact that it
- * must be closed.
- * 2. To avoid "fluent" style composition of functions on Operators. It is important that there never be a set of
- * functional primitives for things like map/filter/reduce to "simplify" the implementation of Operators. This is
- * because such fluency produces really hard to decipher stack traces as the stacktrace ends up being just a bunch
- * of calls from the scaffolding (map/filter/reduce) and not from the actual Operator itself. By not implementing
- * {@code Iterator} we are actively increasing the burden of trying to add such functional operations to the point
- * that hopefully, though code review, we can ensure that we never develop them. It is infinitely better to preserve
- * the stacktrace and "duplicate" the map/filter/reduce scaffolding code.
+ * The other benefit of the go() method is that it fully encapsulates the lifecycle of the underlying resources.
+ * This means that it should be possible to use try/finally blocks around calls to go() in order to ensure that
+ * resources are properly closed.
*/
public interface Operator
{
/**
- * Called to initiate the lifecycle of the Operator. If an operator needs to checkout resources or anything to do
- * its work, this is probably the place to do it.
+ * Tells the Operation to start doing its work. Data will be pushed into the Receiver.
*
- * Work should *never* be done in this method, this method only exists to acquire resources that are known to be
- * needed before doing any work. As a litmus test, if there is ever a call to `op.next()` inside of this method,
- * then something has been done wrong as that call to `.next()` is actually doing work. Such code should be moved
- * into being lazily evaluated as part of a call to `.next()`.
+ * @param receiver a receiver that will receive data
*/
- void open();
+ void go(Receiver receiver);
- /**
- * Returns the next RowsAndColumns object that the Operator can produce. Behavior is undefined if
- * {@link #hasNext} returns false.
- *
- * @return the next RowsAndColumns object that the operator can produce
- */
- RowsAndColumns next();
+ interface Receiver
+ {
+ /**
+ * Used to push data. Return value indicates if more data will be accepted. If false, push should not
+ * be called anymore.
+ *
+ * @param rac {@link RowsAndColumns} of data
+ * @return a boolean value indicating if more data will be accepted. If false, push should never be called
+ * anymore
+ */
+ boolean push(RowsAndColumns rac);
- /**
- * Used to identify if it is safe to call {@link #next}
- *
- * @return true if it is safe to call {@link #next}
- */
- boolean hasNext();
-
- /**
- * Closes this Operator. The cascade flag can be used to identify that the intent is to close this operator
- * and only this operator without actually closing child operators. Other databases us this sort of functionality
- * with a planner that is watching over all of the objects and force-closes even if they were closed during normal
- * operations. In Druid, in the data pipeline where this was introduced, we are guaranteed to always have close
- * called regardless of errors or exceptions during processing, as such, at time of introduction, there is no
- * call that passes false for cascade.
- * <p>
- * That said, given that this is a common thing for these interfaces for other databases, we want to preserve the
- * optionality of being able to leverage what they do. As such, we define the method this way with the belief
- * that it might be used in the future. Semantically, this means that all implementations of Operators must
- * expect to be closed multiple times. I.e. after being closed, it is an error for open, next or hasNext to be
- * called, but close can be called any number of times.
- *
- * @param cascade whether to call close on child operators.
- */
- void close(boolean cascade);
+ /**
+ * Used to indicate that no more data will ever come
+ */
+ void completed();
Review Comment:
is there any contract around what happens if this is called more than once? what happens if i call `push`? is this `close` with a different name? (not to imply i'm advocating for this to be a `Closeable`, i'm really not)
##########
processing/src/main/java/org/apache/druid/query/operator/SegmentToRowsAndColumnsOperator.java:
##########
@@ -36,33 +39,22 @@ public SegmentToRowsAndColumnsOperator(
}
@Override
- public void open()
+ public void go(Receiver receiver)
{
-
- }
-
- @Override
- public RowsAndColumns next()
- {
- hasNext = false;
-
- RowsAndColumns rac = segment.as(RowsAndColumns.class);
- if (rac != null) {
- return rac;
+ try (final CloseableShapeshifter shifty = segment.as(CloseableShapeshifter.class)) {
Review Comment:
it strikes me as a bummer that the segment thing you get here isn't already a `CloseableShapeshifter` to manage the lifecycle of the `RowsAndColumns` to be materialized from it.
I like the `CloseableShapeshifter` pattern, I think maybe we could apply it elsewhere to improve our handling of closeables, because its a bit of a dizzying mess in `Segment` which is why I think this bothers me a bit.
No need to change anything now, just something I want to think about a bit
--
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 #13600: Switch operators to a push-style API
Posted by GitBox <gi...@apache.org>.
github-code-scanning[bot] commented on code in PR #13600:
URL: https://github.com/apache/druid/pull/13600#discussion_r1055441892
##########
processing/src/test/java/org/apache/druid/query/operator/SegmentToRowsAndColumnsOperatorTest.java:
##########
@@ -0,0 +1,187 @@
+/*
+ * 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;
+
+import com.google.common.collect.Lists;
+import org.apache.druid.java.util.common.ISE;
+import org.apache.druid.java.util.common.RE;
+import org.apache.druid.query.operator.window.RowsAndColumnsHelper;
+import org.apache.druid.query.rowsandcols.MapOfColumnsRowsAndColumns;
+import org.apache.druid.query.rowsandcols.RowsAndColumns;
+import org.apache.druid.query.rowsandcols.column.IntArrayColumn;
+import org.apache.druid.segment.ArrayListSegment;
+import org.apache.druid.segment.CloseableShapeshifter;
+import org.apache.druid.segment.TestSegmentForAs;
+import org.apache.druid.segment.column.ColumnType;
+import org.apache.druid.segment.column.RowSignature;
+import org.apache.druid.timeline.SegmentId;
+import org.junit.Assert;
+import org.junit.Test;
+
+import javax.annotation.Nullable;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.concurrent.atomic.AtomicBoolean;
+
+public class SegmentToRowsAndColumnsOperatorTest
+{
+ @Test
+ public void testSanity()
+ {
+ ArrayList<Object[]> rows = Lists.newArrayList(
+ new Object[]{1, 2, "a"},
+ new Object[]{1, 2, "b"}
+ );
+
+ ArrayListSegment<Object[]> segment = new ArrayListSegment<>(
+ SegmentId.dummy("test"),
+ rows,
+ columnName -> objects -> objects[Integer.parseInt(columnName)],
Review Comment:
## Missing catch of NumberFormatException
Potential uncaught 'java.lang.NumberFormatException'.
[Show more details](https://github.com/apache/druid/security/code-scanning/3579)
--
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