clintropolis commented on a change in pull request #12241:
URL: https://github.com/apache/druid/pull/12241#discussion_r802308167
##########
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:
hmm, i thought about this, but only a `DruidRel` of some sort should be
creating a virtual column registry, since they are scoped to a native query, so
this so it didn't seem like a huge deal unless someone is like super super
super deep into custom SQL planning for custom query types (and not fully sure
it is possible without modifying `druid-sql` itself at this point...)
I can't really think of a clean way to add it back; I moved it here because
the expression macro table should not be changing over the course of planning a
query (or even planning multiple queries), so it seemed pointless that we were
passing it in via planner context when we were eagerly creating the virtual
columns (creating expression virtual columns requires a reference to the macro
table), so we can't count on it being set in the constructor we would have to
have an alternative way even for the new methods, which I guess would mean
passing it in when creating an expression and storing a reference to it in each
`DruidExpression` in the tree so that they could build their virtual columns.
I think maybe we should just document this on the unlikely chance someone
was doing something incredibly crazy with this, because in my mind the registry
is a facility that the planner provides to operator
conversions/filters/aggregators etc.
--
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]
