Fix the ORDER BY does not resolve alias from SELECT statement.

[xd1313113]

Relevant Issues

Fix the ORDER BY does not resolve alias from SELECT statement.

Description

Before

SELECT b AS agg_alias FROM [{'b':2},{'b':3}] T GROUP BY b ORDER BY agg_alias DESC;
w: [semantic] Variable reference (agg_alias) could not be found in the database environment or in the set of available locals. Locals: [b].

=== RESULT ===
[
  {
    'agg_alias': 2
  },
  {
    'agg_alias': 3
  }
]

OK!

Now

How it works now is reference in order by will be replaced if there is projection in select.

SELECT a AS b FROM foo ORDER BY b

will be rewritten to

SELECT a AS b FROM foo ORDER BY a

SELECT a + b AS c FROM T ORDER BY c

will be rewritten to 

SELECT a + b AS c FROM T ORDER BY a + b

partiql ▶ SELECT b AS agg_alias FROM [{'b':2},{'b':3}] T GROUP BY b ORDER BY agg_alias DESC;

=== RESULT ===
[
  {
    'agg_alias': 3
  },
  {
    'agg_alias': 2
  }
]

OK!

It also fixed nested query

Select agg_alias as c from (SELECT b AS agg_alias FROM [{'b':2},{'b':3}] T GROUP BY b ORDER BY agg_alias) as T group by c order by c;

New issue Order BY does not work with aggregation function as alias.

SELECT a, MAX(b) as agg_alias FROM [{'a':1, 'b':2},{'a':2, 'b':3}] T GROUP BY a ORDER BY a, agg_alias;

Other Information

• Updated Unreleased Section in CHANGELOG: YES
• Any backward-incompatible changes? NO
• Any new external dependencies? No
• Do your changes comply with the contributing and code style guidelines? YES

License Information

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[github-actions]

CROSS-COMMIT-REPORT-PARTIQLCORE ✅

	BASE (6792E1D)	TARGET (6FEA573)	+/-
% Passing	94.50%	94.69%	0.19% ✅
Passing	6580	6593	13 ✅
Failing	197	188	-9 ✅
Ignored	186	182	-4 ✅
Total Tests	6963	6963	0 ✅

Testing Details

• Base Commit: 6792e1d
• Target Commit: 6fea573
• Java Version: 17.0.16

Result Details

• New passing tests: 0
• New failing tests: 0
• New ignored tests: 0
• Passing in both: 6580
• Failing in both: 188
• Ignored in both: 182
• PASSING in BASE but now FAILING in TARGET: 0
• PASSING in BASE but now IGNORED in TARGET: 0
• FAILING in BASE but now PASSING in TARGET: 9
• IGNORED in BASE but now PASSING in TARGET: 4

Now Passing Tests

The following 9 test(s) were previously FAILING in BASE but are now PASSING in TARGET. Before merging, confirm they are intended to pass:

Click here to see

1. order by select alias, compileOption: PERMISSIVE
2. order by select alias, compileOption: STRICT
3. order by multiple aliases, compileOption: PERMISSIVE
4. order by multiple aliases, compileOption: STRICT
5. order by alias in nested sfw, compileOption: PERMISSIVE
6. order by alias in nested sfw, compileOption: STRICT
7. order by expression alias, compileOption: PERMISSIVE
8. order by expression alias, compileOption: STRICT
9. order by alias with aggregation in nested query, compileOption: STRICT

Now Passing Tests

The following 4 test(s) were previously IGNORED in BASE but are now PASSING in TARGET. Before merging, confirm they are intended to pass:

Click here to see

1. testing alias support, compileOption: PERMISSIVE
2. testing alias support, compileOption: STRICT
3. testing nested alias support, compileOption: PERMISSIVE
4. testing nested alias support, compileOption: STRICT

CROSS-COMMIT-REPORT-PARTIQLEXTENDED ✅

	BASE (6792E1D)	TARGET (6FEA573)	+/-
% Passing	35.22%	35.22%	0.00% ✅
Passing	696	696	0 ✅
Failing	1280	1280	0 ✅
Ignored	0	0	0 ✅
Total Tests	1976	1976	0 ✅

Testing Details

• Base Commit: 6792e1d
• Target Commit: 6fea573
• Java Version: 17.0.16

Result Details

• New passing tests: 0
• New failing tests: 0
• New ignored tests: 0
• Passing in both: 696
• Failing in both: 1280
• Ignored in both: 0
• PASSING in BASE but now FAILING in TARGET: 0
• PASSING in BASE but now IGNORED in TARGET: 0
• FAILING in BASE but now PASSING in TARGET: 0
• IGNORED in BASE but now PASSING in TARGET: 0

[alancai98]

In the prior review, you were using an only ArrayDeque() as the context. Why are we switching to use a parentStack and an aliasMap. From my understanding, it seems like after we exit from some nested context and perform the rewrite over the ORDER BY sets we don't need to keep that alias mapping anymore.

[xd1313113]

The old approach works fine for nested queries but works 'blindly' for set operators as orderby belongs to a querysetExpr without SFW. The new approach provide a better structure awareness context for orderby and allow it to explicitly skip for set op.

[alancai98]

I'm still a bit confused on the changes to the rewriter's context (specifically in this commit 6412350). Previously was an ArrayDeque<MutableMap<String, Expr>> but now you've changed it to a stack and an alias map/list. It seems redundant to store the alias map after visiting an inner SFW where it might not be used anymore in the outer SFW.

The old approach works fine for nested queries but works 'blindly' for set operators as orderby belongs to a querysetExpr without SFW.

What do you mean by "blindly"? There are ways in the visitor pattern to skip over set ops.

[xd1313113]

Let me explain a bit in detail.
In ArrayDeque<MutableMap<String, Expr>> old approach,

Nested query

stack(empty)
ExprQuerySet:(stack(map1))
  body:SFW
    select:
      project 1(add to map1))
    from:
      ExprQuerySet: stack(map1, map2)
        body:SFW
          select:
            project 2 (add to map2))
          from: T
        orderby: Expr 2 (replace from map2)
        stack.pop() // stack(map1)
  orderby:Expr 1 (replace from map1)
stack.pop() // stack(empty)

Setop Query

stack(empty)
ExprQuerySet:(stack(map1))
  body:op
    lhs:ExprQuerySet // stack(map1,map2)
      select:
        project 1(add to map2))
      from: T
    stack.pop()
    rhs:ExprQuerySet stack(map1,map3)
      select:
        project 1(add to map3))
      from: T
    stack.pop() // (stack(map1))
  orderby:Expr 1 (replace from map1) // map1 is empty as there is always no `SELECT`.
stack.pop() // stack(empty)

The reason I swithed to have a parent stack is

• The old approach skipped orderby "blindly" as map is empty and without knowing if it belongs to ExprQuerySet which has setOp body.

There are ways in the visitor pattern to skip over set ops.

• We should not skip setop (inner query main contains order by) but skip order by when its parent has a setop body. The new parentStack approach give some additional information.
• It is true that for now outer SFW does not need alias context of inner SFW. It looks different sql system has different behavior of the final table when set is present. Some uses the result of first SFW as final table schema which means outer SFW can have reference from first SFW. It is quite ambiguous since we do not have partiql specification for set op. I can potentially can use a stack with parent and alias map instead.

[alancai98]

Think we're almost there. Some minor comments