clintropolis commented on code in PR #19711:
URL: https://github.com/apache/druid/pull/19711#discussion_r3648083891
##########
sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidSqlValidator.java:
##########
@@ -536,20 +540,25 @@ private RelDataType validateTargetType(
// disable sealed mode validation if catalog validation is disabled.
final boolean isStrict = tableMetadata.isSealed();
+ // Base table layouts (which require 'sealed') may declare ingest-time
virtual columns; the columns those virtual
+ // columns consume are allowed in the source even though they are not
stored (e.g. a raw column feeding an
+ // expression that computes a stored clustering column).
+ final Set<String> baseTableVirtualColumnInputs =
getBaseTableVirtualColumnInputs(tableMetadata.baseTableMetadata());
Review Comment:
#19659 adds some restrictions to how virtual columns can be used in
clustered segments which make it so that a) all virtual column inputs must be
present as physical columns (or be other virtual columns) and b) all virtual
columns must be an output physical column (or be a dependency of a virtual
column that is an output), which i think makes these cases impossible. It was
weird it was allowed, so i removed it. I was leaning on 🤖 to make tests and at
first glance it seemed ok, but after thinking about it a bit, it doesn't really
make sense to use a virtual column like a transform and then retain it in the
spec definition without its input (and virtual columns that aren't used as an
output to any physical column of the spec are just dead metadata).
After that change, it was much clearer that the validator here needs to
guard against any of the computed columns being inserted to directly as they
are computed by these physical columns, so this logic has been updated
(actually by pushing the 'allowed column' validation into the catalog stuff
itself).
--
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]
