[GitHub] [druid] paul-rogers commented on a diff in pull request #13173: Fix row schema of nested queries with ORDER BY clause to get proper row type during planning validation

[GitBox <gi...@apache.org>]

paul-rogers commented on code in PR #13173:
URL: https://github.com/apache/druid/pull/13173#discussion_r991528258


##########
sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidOuterQueryRel.java:
##########
@@ -116,12 +114,20 @@ public DruidQuery toDruidQuery(final boolean finalizeAggregations)
   @Override
   public DruidQuery toDruidQueryForExplaining()
   {
+    final RowSignature sourceRowSignature;
+    if (sourceRel instanceof DruidRel) {
+      final DruidQuery subQuery = ((DruidRel) sourceRel).toDruidQueryForExplaining();
+      sourceRowSignature = subQuery.getOutputRowSignature();
+    } else {
+      // fallback for cases other than group by inner query with order by with alias on __time column
+      sourceRowSignature = RowSignatures.fromRelDataType(
+          sourceRel.getRowType().getFieldNames(),
+          sourceRel.getRowType()

Review Comment:
   If aliasing is used, Calcite should ensure that the signature of the inner `SELECT` provides the alias names. If not, we broke something since this is pretty basic SQL functionality.
   
   If, on the other hand, we want the column name `__time`, even if the user said `__time AS t`, then we're using non-standard SQL behavior. We can unpack the alias to work back to the base column if we need to know that this is the `__time` column. However, we should not need to: the outer query should not, in general, special-case `__time`. Our implementation might, given that native queries are focused on the event use case.
   
   Suppose we have to know that `t` is really `__time`. The proper thing to do is to check if the column is an alias. If so, get the base column as above. It may also be possible to attach traits to the column. Columns an have traits such as their cardinality, etc. We might be able to add a trait saying the column kind (`__time`, dimension, measure or whatever.)



##########
sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidOuterQueryRel.java:
##########
@@ -116,12 +114,20 @@ public DruidQuery toDruidQuery(final boolean finalizeAggregations)
   @Override
   public DruidQuery toDruidQueryForExplaining()
   {
+    final RowSignature sourceRowSignature;
+    if (sourceRel instanceof DruidRel) {
+      final DruidQuery subQuery = ((DruidRel) sourceRel).toDruidQueryForExplaining();
+      sourceRowSignature = subQuery.getOutputRowSignature();
+    } else {
+      // fallback for cases other than group by inner query with order by with alias on __time column
+      sourceRowSignature = RowSignatures.fromRelDataType(
+          sourceRel.getRowType().getFieldNames(),
+          sourceRel.getRowType()

Review Comment:
   Another concern. If we return as the row signature the signature of the source, we may miss computed columns:
   
   ```sql
   SELECT * FROM
     SELECT __time AS t, myCol as c, a + b AS d FROM foo
   ```
   
   Here, the signature of the inner query is `(t TIMESTAMP, c VARCHAR, d BIGINT)`, say. If we get the source signature (based on `foo`), we would omit the computed column `d`, which is likely to cause cascading issues.
   
   Let's be sure to test this case to validate the workaround.



-- 
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