Skip to content

Commit

Permalink
Fixes group by for more than 2 nested path expressions. (#461)
Browse files Browse the repository at this point in the history
* Fixes group by for more than 2 nested path expressions.

* Renames test description and refines kdoc.
  • Loading branch information
abhikuhikar authored Oct 27, 2021
1 parent 9427146 commit c88b3cf
Show file tree
Hide file tree
Showing 2 changed files with 16 additions and 8 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,9 @@ class GroupByPathExpressionVisitorTransform(
/**
* Determines if [gbi] is an expression of the type that should be replaced elsewhere in the query.
*
* Since we are only concerned about SQL-92 compatibility here, we only replace path expressions that
* have a single component, i.e. `f.bar` but not `f.bar.bat`. (The latter is not part of SQL-92.)
* As we support querying nested data, we are not just limited to SQL-92 compatibility (that have
* path expressions with single component, i.e. `foo.bar`) but also replace path expressions that
* have more than one component, i.e. `f.bar.bat`.
*/
fun canBeSubstituted(groupKey: PartiqlAst.GroupKey): Boolean {
val expr = groupKey.expr
Expand All @@ -42,15 +43,12 @@ class GroupByPathExpressionVisitorTransform(
return when {
asName == null -> throw IllegalStateException("GroupByItem.asName must be specified for this transform to work")
!asName.metas.containsKey(IsSyntheticNameMeta.TAG) ->
// If this meta is not present it would indicate that the alias was explicitly specifed, which is
// If this meta is not present it would indicate that the alias was explicitly specified, which is
// not allowed by SQL-92, so ignore.
false

// Group by expressions other than paths aren't part of SQL-92 so ignore
expr !is PartiqlAst.Expr.Path -> false

// Path expressions containing more than one component (i.e. `one.two.three`) are also not part of SQL-92
expr.steps.size != 1 -> false
else -> true
}

Expand Down
14 changes: 12 additions & 2 deletions lang/test/org/partiql/lang/eval/EvaluatingCompilerGroupByTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,13 @@ EvaluatingCompilerGroupByTest : EvaluatorTestBase() {
{ customerId: 789, sellerId: 2, productId: 88888, cost: 8 },
{ customerId: 789, sellerId: 1, productId: 99999, cost: 9 },
{ customerId: 100, sellerId: 2, productId: 10000, cost: 10 }
]""").toSession()
]""",
"employees" to """[
{ name: 'Joey', age: 25, manager: { name: 'John', address: { city: 'Seattle' } } },
{ name: 'Chandler', age: 27, manager: { name: 'Rocky', address: { city: 'Seattle' } } },
{ name: 'Ross', age: 22, manager: { 'name': 'Alex', address: { city: 'Chicago' } } }
]"""
).toSession()

companion object {

Expand Down Expand Up @@ -720,7 +726,11 @@ EvaluatingCompilerGroupByTest : EvaluatorTestBase() {
EvaluatorTestCase(
"SELECT VALUE with nested aggregates",
"SELECT VALUE (SELECT SUM(outerFromSource.col1) AS the_sum FROM <<1>>) FROM simple_1_col_1_group as outerFromSource",
"<< << { 'the_sum': 1 } >>, << { 'the_sum': 1 } >> >>")
"<< << { 'the_sum': 1 } >>, << { 'the_sum': 1 } >> >>"),
EvaluatorTestCase(
"SELECT with GROUP BY path expression having more than 1 component.",
"SELECT avg(age) as avg_employee_age, manager.address.city FROM employees GROUP BY manager.address.city",
"<<{'avg_employee_age': 22, 'city': 'Chicago'}, {'avg_employee_age': 26, 'city': 'Seattle'}>>")
)

@Test
Expand Down

0 comments on commit c88b3cf

Please sign in to comment.