imply-cheddar commented on a change in pull request #12241:
URL: https://github.com/apache/druid/pull/12241#discussion_r802250747
##########
File path:
extensions-core/datasketches/src/test/java/org/apache/druid/query/aggregation/datasketches/quantiles/sql/DoublesSketchSqlAggregatorTest.java
##########
@@ -564,7 +564,7 @@ public void testDoublesSketchPostAggs() throws Exception
),
new ExpressionPostAggregator(
"p3",
- "(p2 + 1000)",
+ "(\"p2\" + 1000)",
Review comment:
Naive question, but is this test change indicative of a change in what
SQL is required to be written? I.e. might some things that didn't require
quotes previously suddenly require quotes?
##########
File path:
sql/src/main/java/org/apache/druid/sql/calcite/expression/DruidExpression.java
##########
@@ -32,127 +32,303 @@
import org.apache.druid.segment.virtual.ExpressionVirtualColumn;
import javax.annotation.Nullable;
+import java.util.ArrayList;
import java.util.Arrays;
+import java.util.Collections;
import java.util.List;
import java.util.Objects;
import java.util.function.Function;
/**
+ * todo(clint): rewrite javadocs
+ *
* Represents two kinds of expression-like concepts that native Druid queries
support:
*
* (1) SimpleExtractions, which are direct column access, possibly with an
extractionFn
* (2) native Druid expressions.
*/
public class DruidExpression
{
+ public enum NodeType
+ {
+ /**
+ * constant value
+ */
+ LITERAL,
+ /**
+ * Identifier for a direct physical or virtual column access (column name
or virtual column name)
+ */
+ IDENTIFIER,
+ /**
+ * Standard native druid expression, which can compute a string that can
be parsed into {@link Expr}, or used
+ * as an {@link ExpressionVirtualColumn}
+ */
+ EXPRESSION,
+ /**
+ * Expression backed by a specialized {@link VirtualColumn}, which might
provide more optimized evaluation than
+ * is possible with the standard
+ */
+ SPECIALIZED
+ }
+
// Must be sorted
private static final char[] SAFE_CHARS = "
,._-;:(){}[]<>!@#$%^&*`~?/".toCharArray();
+ private static final VirtualColumnBuilder DEFAULT_VIRTUAL_COLUMN_BUILDER =
new ExpressionVirtualColumnBuilder();
Review comment:
I'm scared of static buildery thingies. Is there a reason that this
needs to be static instead of just creating a new one each time it's used?
##########
File path:
sql/src/main/java/org/apache/druid/sql/calcite/expression/DruidExpression.java
##########
@@ -32,127 +32,303 @@
import org.apache.druid.segment.virtual.ExpressionVirtualColumn;
import javax.annotation.Nullable;
+import java.util.ArrayList;
import java.util.Arrays;
+import java.util.Collections;
import java.util.List;
import java.util.Objects;
import java.util.function.Function;
/**
+ * todo(clint): rewrite javadocs
+ *
* Represents two kinds of expression-like concepts that native Druid queries
support:
*
* (1) SimpleExtractions, which are direct column access, possibly with an
extractionFn
* (2) native Druid expressions.
*/
public class DruidExpression
{
+ public enum NodeType
+ {
+ /**
+ * constant value
+ */
+ LITERAL,
+ /**
+ * Identifier for a direct physical or virtual column access (column name
or virtual column name)
+ */
+ IDENTIFIER,
+ /**
+ * Standard native druid expression, which can compute a string that can
be parsed into {@link Expr}, or used
+ * as an {@link ExpressionVirtualColumn}
+ */
+ EXPRESSION,
+ /**
+ * Expression backed by a specialized {@link VirtualColumn}, which might
provide more optimized evaluation than
+ * is possible with the standard
+ */
+ SPECIALIZED
+ }
+
// Must be sorted
private static final char[] SAFE_CHARS = "
,._-;:(){}[]<>!@#$%^&*`~?/".toCharArray();
+ private static final VirtualColumnBuilder DEFAULT_VIRTUAL_COLUMN_BUILDER =
new ExpressionVirtualColumnBuilder();
static {
Arrays.sort(SAFE_CHARS);
}
- @Nullable
- private final SimpleExtraction simpleExtraction;
- private final String expression;
- private final TrinaryFn<String, ColumnType, ExprMacroTable, VirtualColumn>
virtualColumnFn;
+ private static String escape(final String s)
+ {
+ final StringBuilder escaped = new StringBuilder();
+ for (int i = 0; i < s.length(); i++) {
+ final char c = s.charAt(i);
+ if (Character.isLetterOrDigit(c) || Arrays.binarySearch(SAFE_CHARS, c)
>= 0) {
+ escaped.append(c);
+ } else {
+
escaped.append("\\u").append(BaseEncoding.base16().encode(Chars.toByteArray(c)));
+ }
+ }
+ return escaped.toString();
+ }
- private DruidExpression(@Nullable final SimpleExtraction simpleExtraction,
final String expression, @Nullable final TrinaryFn<String, ColumnType,
ExprMacroTable, VirtualColumn> virtualColumnFn)
Review comment:
The diff is making it hard to really see this, but I *think* you got rid
of this 3-argument constructor. That's going to make anything built on the old
code stop working. Is it possible to have the 3-argument constructor continue
to exist, but be deprecated and delegate to the new constructor kinda like you
did with the other methods?
##########
File path:
sql/src/main/java/org/apache/druid/sql/calcite/rel/VirtualColumnRegistry.java
##########
@@ -41,29 +44,33 @@
*/
public class VirtualColumnRegistry
{
+ private final ExprMacroTable macroTable;
private final RowSignature baseRowSignature;
- private final Map<ExpressionWrapper, VirtualColumn>
virtualColumnsByExpression;
- private final Map<String, VirtualColumn> virtualColumnsByName;
+ private final Map<ExpressionAndTypeHint, String> virtualColumnsByExpression;
+ private final Map<String, ExpressionAndTypeHint> virtualColumnsByName;
private final String virtualColumnPrefix;
private int virtualColumnCounter;
private VirtualColumnRegistry(
RowSignature baseRowSignature,
+ ExprMacroTable macroTable,
String virtualColumnPrefix,
- Map<ExpressionWrapper, VirtualColumn> virtualColumnsByExpression,
- Map<String, VirtualColumn> virtualColumnsByName
+ Map<ExpressionAndTypeHint, String> virtualColumnsByExpression,
+ Map<String, ExpressionAndTypeHint> virtualColumnsByName
)
{
+ this.macroTable = macroTable;
this.baseRowSignature = baseRowSignature;
this.virtualColumnPrefix = virtualColumnPrefix;
this.virtualColumnsByExpression = virtualColumnsByExpression;
this.virtualColumnsByName = virtualColumnsByName;
}
- public static VirtualColumnRegistry create(final RowSignature rowSignature)
Review comment:
This single-argument `create` was not resurrected. I haven't double
checked if that might be a compatibility problem or not, but just wanted to
highlight it.
##########
File path:
sql/src/main/java/org/apache/druid/sql/calcite/expression/DruidExpression.java
##########
@@ -32,127 +32,303 @@
import org.apache.druid.segment.virtual.ExpressionVirtualColumn;
import javax.annotation.Nullable;
+import java.util.ArrayList;
import java.util.Arrays;
+import java.util.Collections;
import java.util.List;
import java.util.Objects;
import java.util.function.Function;
/**
+ * todo(clint): rewrite javadocs
Review comment:
nit: you've got a todo todo
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
