airframe-sql: Fixes #2646 Resolve AllColumns inputs as ResolvedAttributes #2649

xerial · 2022-12-17T08:54:20Z

Resolve FunctionCall input as a ResolvedAttribute to avoid too eager pull-up of FunctionCall airframe-sql: Invalid aggregation key propagation #2646

An example plan after this fix:

To resolve arbitrary aggregation function data type, we need to provide function tables. When no function table is available, resolving its DataType as name: ? (Unknown type) is ok. count(...) always returns Long, so added a quick path.
I think we don't need to propagate detailed expressions like FunctionCall to upper nodes. For example, the following item (iii) can be ResolvedAttribute with source column information:
1. table scan (source columns: c1, c2, ...)
2. function call (source columns: c1, c2, ...)
3. reference to the result of (ii) (source columns: c1, c2, ...)

codecov · 2022-12-17T09:00:00Z

Codecov Report

Merging #2649 (900ae00) into master (6b766a6) will increase coverage by 0.01%.
The diff coverage is 87.75%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2649      +/-   ##
==========================================
+ Coverage   82.12%   82.14%   +0.01%     
==========================================
  Files         334      334              
  Lines       14030    14050      +20     
  Branches     2182     2213      +31     
==========================================
+ Hits        11522    11541      +19     
- Misses       2508     2509       +1

Impacted Files	Coverage Δ
.../scala/wvlet/airframe/sql/model/ResolvedPlan.scala	`56.25% <0.00%> (ø)`
...scala/wvlet/airframe/sql/parser/SQLGenerator.scala	`91.03% <ø> (ø)`
.../wvlet/airframe/sql/model/LogicalPlanPrinter.scala	`96.55% <50.00%> (-3.45%)`	⬇️
...in/scala/wvlet/airframe/sql/model/Expression.scala	`67.20% <75.00%> (+1.09%)`	⬆️
...ala/wvlet/airframe/sql/analyzer/TypeResolver.scala	`93.46% <100.00%> (-0.16%)`	⬇️
...n/scala/wvlet/airframe/sql/model/LogicalPlan.scala	`89.43% <100.00%> (+0.03%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6b766a6...900ae00. Read the comment docs.

xerial · 2022-12-17T10:33:49Z

airframe-sql/src/main/scala/wvlet/airframe/sql/analyzer/TypeResolver.scala

-          case expr => if (isSelectItem) SingleColumn(expr, Some(i.value), None, expr.nodeLocation) else expr
+          case f: FunctionCall =>
+            // Extract source columns
+            val src = Set.newBuilder[SourceColumn]


SourceColumn contains a TableCatalog parameter, so using Set operation might be a bit heavy if the table schema is large.

takezoe · 2022-12-17T11:37:11Z

airframe-sql/src/main/scala/wvlet/airframe/sql/model/Expression.scala

-    // override def dataType: DataType = super.dataType
+    override def dataType: DataType = {
+      if (functionName == "count") {
+        DataType.LongType


takezoe · 2022-12-17T11:52:13Z

airframe-sql/src/test/scala/wvlet/airframe/sql/analyzer/TypeResolverTest.scala

          }
+          col shouldMatch { case ResolvedAttribute("cnt", DataType.LongType, _, sourceColumns, _) => }


Just wondering, what's sourceColumns here? It might be better to verify.

I think the sourceColumns need to be the all of the column data used for generating this ResolvedAttribute. If we use airframe-sql for tracking data lineage, we may need to maintain this property across SQL expression nodes.

For example, if the function is max/min/max_by/min_by etc., source column information will be necessary to show the result contains any PID. If the function is count(1), personal information will be lost, so sourceColumns might be unnecessary because raw column values will not be exposed.

sourceColumns seems an empty List in this pull request. Is this expected?

How about defining sourceColumns as input columns without any modifications so that data lineage with nested function calls, such as count(max(..(col))), can be traced in a different manner outside TypeResolver?

If it sounds good, I will fix code and tests accordingly. In this case, cnt is from count(), so sourceColumn should be empty, but the function argument (AllColumns) of FunctionCall("count", Seq(AllColumns(, sourceColumns(...)) ) ) should have a source column.

It looks like AllColumns (in SELECT clause) isn't resolved before cnt (in HAVING clause) is resolved.

I noticed that this query is actually invalid on Presto:

select id, count(*) cnt from A group by id having cnt > 10

Got the following error:

Query 20221219_122609_00005_r3q5y failed: line 1:84: Column 'cnt' cannot be resolved

Meaning we don't need to resolve cnt in HAVING clause in this case.

Good catch 😅

Fixed a bug in resolve HAVING clause airframe-sql: Fixes #2646 Resolve AllColumns inputs as ResolvedAttributes #2649 (comment)

Make explicit that ResolvedAttribute.sourceColumns is given only when the attribute is a direct reference of table columns without any change (e.g., via function). https://github.com/wvlet/airframe/pull/2649/files#diff-5f3b4602b1d9bc6f2aeff669d9b32b2a17d1e369c856d09b295eac7de8118c97R63-R64

Make explicit that ResolvedAttribute.sourceColumns is given only when the attribute is a direct reference of table columns without any change

Oh, I see.

takezoe · 2022-12-17T12:08:33Z

airframe-sql/src/main/scala/wvlet/airframe/sql/analyzer/TypeResolver.scala

@@ -379,7 +385,27 @@ object TypeResolver extends LogSupport {
          case a: Attribute   => a
          case m: MultiColumn => m
          // retain alias for select column
-          case expr => if (isSelectItem) SingleColumn(expr, Some(i.value), None, expr.nodeLocation) else expr
+          case f: FunctionCall =>


Wondering if the same way can be applied to any expression not only FunctionCall. 🤔

That's also my concern. Maybe converting every type of expression other than already resolved Attribute/MultiColumn as ResolvedAttribute?

I'm not 100% but maybe that should be.

Marked as TODO for now

Resolved every matched expressions as ResolvedAttribute here

xerial · 2022-12-19T19:25:43Z

airframe-sql/src/main/scala/wvlet/airframe/sql/analyzer/TypeResolver.scala

            GroupingKey(e, e.nodeLocation)
          })
        val resolvedHaving = having.map {
          _.transformUpExpression { case x: Expression =>
-            resolveExpression(context, x, a.outputAttributes, false)
+            // Having recognize attributes only from the input relation
+            resolveExpression(context, x, childOutputAttributes, false)


This part had a bug, which was found in
#2649 (comment)

xerial · 2022-12-19T23:11:10Z

airframe-sql/src/main/scala/wvlet/airframe/sql/model/LogicalPlanPrinter.scala

+
+        val prefix = m match {
+          case t: TableScan =>
+            s"${ws}[${m.modelName}] ${t.table.fullName}${functionSig}"


For showing the input table name after TableRef is resolved to TableScan

xerial · 2022-12-19T23:11:23Z

airframe-sql/src/main/scala/wvlet/airframe/sql/model/ResolvedPlan.scala

@@ -97,7 +99,7 @@ case class ResolvedAttribute(
      case (Some(q), columns) if columns.nonEmpty =>
        columns
          .map(_.fullName)
-          .mkString(s"${q},${typeDescription} <- [", ", ", "]")
+          .mkString(s"${q}.${typeDescription} <- [", ", ", "]")


Fixed a typo

xerial · 2022-12-20T00:06:39Z

airframe-sql/src/main/scala/wvlet/airframe/sql/analyzer/TypeResolver.scala

            GroupingKey(e, e.nodeLocation)
          })
        val resolvedHaving = having.map {
          _.transformUpExpression { case x: Expression =>
-            resolveExpression(context, x, a.outputAttributes, false)
+            // Having recognize attributes only from the input relation
+            resolveExpression(context, x, childOutputAttributes)


This needed to use the resolved child attributes

takezoe

LGTM

xerial · 2022-12-20T00:08:03Z

airframe-sql/src/main/scala/wvlet/airframe/sql/analyzer/TypeResolver.scala

@@ -324,15 +331,14 @@ object TypeResolver extends LogSupport {
  private def findMatchInInputAttributes(
      context: AnalyzerContext,
      expr: Expression,
-      inputAttributes: Seq[Attribute],
-      isSelectItem: Boolean


MultiColumn is changed to Attribute, so we can use it as SelectItem as is.

xerial added 4 commits December 16, 2022 22:26

airframe-sql: Add RewriteRule to see TypeResolver behavior

899e79a

Fix scope

890a806

Suppress second resolver logs

467b215

airframe-sql: Fixes #2646 for count(*) queries

d62db6c

xerial changed the base branch from master to agg-fix December 17, 2022 08:54

github-actions bot added the bug label Dec 17, 2022

xerial changed the title ~~agg resolver fix~~ airframe-sql: Fixes #2646 for count(*) queries Dec 17, 2022

xerial added 4 commits December 17, 2022 01:41

Propagate source columns from FunctionCall

fec989e

fix

5245753

Check resolved attributes

df11362

Use robust match

1029db4

xerial requested a review from takezoe December 17, 2022 10:31

xerial commented Dec 17, 2022

View reviewed changes

takezoe reviewed Dec 17, 2022

View reviewed changes

Base automatically changed from agg-fix to master December 17, 2022 19:39

xerial added 5 commits December 17, 2022 14:25

Merge branch 'master' into agg-resolver-fix

31ad0ce

Fix the input attributes for resolving HAVING

dbd4534

Mark TODO

94f3dfd

ResolvedAttribute of function no need to maintain source columns

4dbf48f

Fix var name

86b8d4e

xerial commented Dec 19, 2022

View reviewed changes

xerial added 2 commits December 19, 2022 13:16

Resolve all matched expressions as ResolvedAttribute

1404cda

Resolve input attributes appropriately

1c299a1

xerial changed the title ~~airframe-sql: Fixes #2646 for count(*) queries~~ airframe-sql: Fixes #2646 Resolve input parameters as ResolvedAttributes Dec 19, 2022

Make MultiColumn an Attribute

900ae00

xerial commented Dec 19, 2022

View reviewed changes

xerial changed the title ~~airframe-sql: Fixes #2646 Resolve input parameters as ResolvedAttributes~~ airframe-sql: Fixes #2646 Resolve AllColumns inputs as ResolvedAttributes Dec 19, 2022

xerial commented Dec 20, 2022

View reviewed changes

takezoe approved these changes Dec 20, 2022

View reviewed changes

xerial commented Dec 20, 2022

View reviewed changes

xerial merged commit 7c0616e into master Dec 20, 2022

xerial deleted the agg-resolver-fix branch December 20, 2022 00:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

airframe-sql: Fixes #2646 Resolve AllColumns inputs as ResolvedAttributes #2649

airframe-sql: Fixes #2646 Resolve AllColumns inputs as ResolvedAttributes #2649

xerial commented Dec 17, 2022 •

edited

Loading

codecov bot commented Dec 17, 2022 •

edited

Loading

xerial Dec 17, 2022 •

edited

Loading

takezoe Dec 17, 2022

takezoe Dec 17, 2022

xerial Dec 17, 2022

xerial Dec 17, 2022

takezoe Dec 18, 2022

xerial Dec 18, 2022 •

edited

Loading

takezoe Dec 19, 2022 •

edited

Loading

takezoe Dec 19, 2022 •

edited

Loading

xerial Dec 19, 2022

xerial Dec 19, 2022 •

edited

Loading

takezoe Dec 20, 2022

takezoe Dec 17, 2022

xerial Dec 17, 2022 •

edited

Loading

takezoe Dec 18, 2022

xerial Dec 19, 2022

xerial Dec 19, 2022

xerial Dec 19, 2022

xerial Dec 19, 2022

xerial Dec 19, 2022

xerial Dec 20, 2022

takezoe left a comment

xerial Dec 20, 2022

		}
		col shouldMatch { case ResolvedAttribute("cnt", DataType.LongType, _, sourceColumns, _) => }

airframe-sql: Fixes #2646 Resolve AllColumns inputs as ResolvedAttributes #2649

airframe-sql: Fixes #2646 Resolve AllColumns inputs as ResolvedAttributes #2649

Conversation

xerial commented Dec 17, 2022 • edited Loading

codecov bot commented Dec 17, 2022 • edited Loading

Codecov Report

xerial Dec 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xerial Dec 18, 2022 • edited Loading

Choose a reason for hiding this comment

takezoe Dec 19, 2022 • edited Loading

Choose a reason for hiding this comment

takezoe Dec 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xerial Dec 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xerial Dec 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

takezoe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xerial commented Dec 17, 2022 •

edited

Loading

codecov bot commented Dec 17, 2022 •

edited

Loading

xerial Dec 17, 2022 •

edited

Loading

xerial Dec 18, 2022 •

edited

Loading

takezoe Dec 19, 2022 •

edited

Loading

takezoe Dec 19, 2022 •

edited

Loading

xerial Dec 19, 2022 •

edited

Loading

xerial Dec 17, 2022 •

edited

Loading