Add pair Latest/Earliest aggregators by imply-cheddar · Pull Request #14195 · apache/druid

imply-cheddar · 2023-05-02T05:19:53Z

The current Latest/Earliest aggregators all do
finalization to pretend like they are dealing with their payload type instead of exposing that they are actually dealing with pairs. This adds a STRING_LATEST and STRING_EARLIEST that explicitly deal with pairs and also adds PAIR_LEFT and PAIR_RIGHT functions that can access the two sides of the pairs.

This does not add the numerical versions yet, though that can be an easy iteration on top of this.

The current Latest/Earliest aggregators all do finalization to pretend like they are dealing with their payload type instead of exposing that they are actually dealing with pairs. This adds a STRING_LATEST and STRING_EARLIEST that explicitly deal with pairs and also adds PAIR_LEFT and PAIR_RIGHT functions that can access the two sides of the pairs. This does not add the numerical versions yet, though that can be an easy iteration on top of this.

+            return ExpressionType.LONG;
+          }
+        }
+        throw new UOE("Invalid input type [%s] to pair_left, expected a complex pair type");


somu-imply · 2023-06-03T02:06:59Z

+            return ExpressionType.STRING;
+          }
+        }
+        throw new UOE("Invalid input type [%s] to pair_right, expected a complex pair type");


Both the missing format arguments need to be introduced

somu-imply · 2023-06-03T01:50:21Z

+
+import javax.annotation.Nullable;
+
+public class PairOperatorConversion implements SqlOperatorConversion


Can this implement DirectOperatorConversion instead ?

somu-imply · 2023-06-03T01:59:34Z

+    validationHelperCheckArgumentCount(args, 1);
+    Expr arg = args.get(0);
+
+    class PairLeft extends ExprMacroTable.BaseScalarUnivariateMacroFunctionExpr


nit. Might call it PairLeftExpression

Also, we can move these to individual classes

clintropolis · 2023-08-15T19:10:38Z

+      @JsonProperty("fieldName") @Nullable final String fieldName,
      @JsonProperty("timeColumn") @Nullable final String timeColumn,
-      @JsonProperty("maxStringBytes") Integer maxStringBytes
+      @JsonProperty("foldColumn") @Nullable final String foldColumn,


why do we need a separate foldColumn? It seems like instead we could just not auto initialize the timeColumn, and in factorize methods examine the ColumnCapabilities of ColumnSelectorFactory of the fieldName column and factorize into either the folding aggregator or the building aggregator as appropriate

clintropolis · 2023-08-15T19:12:03Z

+    // Less efficient code path when folding is a possibility (we must read the value selector first just in case
+    // it's a foldable object).


nit: stale comment copied from old combined agg?

clintropolis · 2023-08-15T19:14:02Z

+      @JsonProperty("fieldName") @Nullable final String fieldName,
      @JsonProperty("timeColumn") @Nullable final String timeColumn,
-      @JsonProperty("maxStringBytes") Integer maxStringBytes
+      @JsonProperty("foldColumn") @Nullable final String foldColumn,


same comment about foldColumn vs just using ColumnCapabilities

clintropolis · 2023-08-15T19:19:54Z

+      @Override
+      public ExpressionType getOutputType(InputBindingInspector inspector)
+      {
+        final ExpressionType inputType = inspector.getType(arg.getBindingIfIdentifier());


inputType could be null here. You could alternatively use arg.getOutputType(inspector) though it can also still return null.

If the argument must be an identifier, i would recommend pulling arg.getBindingIfIdentifier() and validating that it is not null separately.

clintropolis · 2023-08-15T19:22:18Z

+            return ExpressionType.LONG;
+          }
+        }
+        throw new UOE("Invalid input type [%s] to pair_left, expected a complex pair type");


macro expression implement an interface called NamedFunction that provide a bunch of validation helper methods to help provide consistent error messaging for expressions, so could do something like

validationFailed("Invalid input type [%s], expected [%s]", inputType, EXPRESSION_TYPE)

clintropolis · 2023-08-15T19:23:55Z

+      @Override
+      public ExpressionType getOutputType(InputBindingInspector inspector)
+      {
+        final ExpressionType inputType = inspector.getType(arg.getBindingIfIdentifier());


same comment about using arg.getOutputType(inspector)

clintropolis · 2023-08-15T19:28:31Z

  public static final byte KLL_FLOATS_SKETCH_TO_QUANTILE_CACHE_TYPE_ID = 42;
  public static final byte KLL_FLOATS_SKETCH_TO_QUANTILES_CACHE_TYPE_ID = 43;
  public static final byte KLL_FLOATS_SKETCH_TO_STRING_CACHE_TYPE_ID = 44;
+  public static final byte PAIRS = 45;


this isn't needed?

clintropolis · 2023-08-15T19:32:48Z

          case STRING:
          case COMPLEX:
-            return new StringFirstAggregatorFactory(name, fieldName, timeColumn, maxStringBytes);
+            return new StringFirstAggregatorFactory(name, fieldName, timeColumn, null, maxStringBytes, true);


is this right if we go forward with separate foldColumn? Shouldn't the complex case put fieldName in the foldColumn?

If you drop foldColumn and use the selector factory + capabilities (my preference at least at the time of writing this comment) instead then this doesn't need to change for now I think. Though, this part will also need to change if the PR that adds ingest time support for numeric first/last, since we'll need to get the complex type name and pick the correct folding aggregator...

clintropolis · 2023-08-15T19:33:04Z

          case STRING:
          case COMPLEX:
-            return new StringLastAggregatorFactory(name, fieldName, timeColumn, maxStringBytes);
+            return new StringLastAggregatorFactory(name, fieldName, timeColumn, null, maxStringBytes, true);


same comment about COMPLEX case and foldColumn.

clintropolis · 2023-08-15T19:40:17Z

+public class EarliestLatestPairSqlAggregator implements SqlAggregator
+{
+  public static final SqlAggregator[] DEFINED_AGGS = new SqlAggregator[]{
+      new EarliestLatestPairSqlAggregator(AggregatorType.EARLIEST, PayloadType.STRING),
+      new EarliestLatestPairSqlAggregator(AggregatorType.LATEST, PayloadType.STRING)
+  };
+
+  enum AggregatorType
+  {
+    EARLIEST, LATEST
+  }
+
+  enum PayloadType
+  {
+    STRING, LONG, DOUBLE
+  }


do we really need new functions to do aggregations on complex types? can't EARLIEST and LATEST detect when their inputs are a pair and do the right thing?

github-actions · 2024-02-12T00:17:19Z

This pull request has been marked as stale due to 60 days of inactivity.
It will be closed in 4 weeks if no further activity occurs. If you think
that's incorrect or this pull request should instead be reviewed, please simply
write any comment. Even if closed, you can still revive the PR at any time or
discuss it on the dev@druid.apache.org list.
Thank you for your contributions.

github-actions · 2024-03-12T00:15:41Z

This pull request/issue has been closed due to lack of activity. If you think that
is incorrect, or the pull request requires review, you can revive the PR at any time.

github-actions · 2024-05-21T00:17:02Z

This pull request has been marked as stale due to 60 days of inactivity.
It will be closed in 4 weeks if no further activity occurs. If you think
that's incorrect or this pull request should instead be reviewed, please simply
write any comment. Even if closed, you can still revive the PR at any time or
discuss it on the dev@druid.apache.org list.
Thank you for your contributions.

github-actions · 2024-06-18T00:17:43Z

This pull request/issue has been closed due to lack of activity. If you think that
is incorrect, or the pull request requires review, you can revive the PR at any time.

github-advanced-security AI found potential problems May 3, 2023

View reviewed changes

somu-imply reviewed Jun 3, 2023

View reviewed changes

jon-wei mentioned this pull request Jun 21, 2023

Support complex variance object inputs for variance SQL agg function #14463

Merged

9 tasks

clintropolis reviewed Aug 15, 2023

View reviewed changes

clintropolis mentioned this pull request Aug 15, 2023

enable sql compatible null handling mode by default #14792

Merged

4 tasks

cheddar mentioned this pull request Jan 12, 2024

Update the docs for EARLIEST_BY/LATEST_BY aggregators with the newly added numeric capabilities #15670

Merged

10 tasks

github-actions Bot added the stale label Feb 12, 2024

github-actions Bot closed this Mar 12, 2024

LakshSingla reopened this Mar 21, 2024

github-actions Bot added Area - Querying and removed stale labels Mar 21, 2024

github-actions Bot added the stale label May 21, 2024

github-actions Bot closed this Jun 18, 2024


		import javax.annotation.Nullable;

		public class PairOperatorConversion implements SqlOperatorConversion

		// Less efficient code path when folding is a possibility (we must read the value selector first just in case
		// it's a foldable object).

Conversation

imply-cheddar commented May 2, 2023

Uh oh!

Check failure

Check failure

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Feb 12, 2024

Uh oh!

github-actions Bot commented Mar 12, 2024

Uh oh!

github-actions Bot commented May 21, 2024

Uh oh!

github-actions Bot commented Jun 18, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants