fix: handle group_by identifier more generically #127

kotharironak · 2022-09-05T04:34:49Z

The previous fix - #124 for the issue #123, was handling a very specific case.

As part of this PR,

it makes it more generic, and make sure that all the groupby clause identifier match with selection clause identifiers.

Testing:

all existing tests are passing
added new unit + integration test

Example of handling same in existing mongo impl:
Before transformation:

SELECT DISTINCT_COUNT(`quantity`) AS qty_count, `item`, `price` FROM <implicit_collection> WHERE `price` <= 10 GROUP BY `item`

After transformation:

SELECT DISTINCT(`quantity`) AS qty_count, `_id.item` AS item, `price`, LENGTH(`qty_count`) AS qty_count FROM <implicit_collection> WHERE `price` <= 10 GROUP BY `item`

Mongo Query:

[{"$match": {"price": {"$lte": 10}}}, 
{"$group": {"qty_count": {"$addToSet": "$quantity"}, "_id": {"item": "$item"}}}, 
{"$project": {"item": "$_id.item", "qty_count": {"$size": "$qty_count"}, "price": 1}}]

Response:

[{"item":"Soap","qty_count":2},{"item":"Comb","qty_count":2},{"item":"Shampoo","qty_count":2}]

codecov · 2022-09-05T04:37:42Z

Codecov Report

Merging #127 (d5e3360) into main (30c69aa) will increase coverage by 0.10%.
The diff coverage is 90.62%.

@@             Coverage Diff              @@
##               main     #127      +/-   ##
============================================
+ Coverage     78.88%   78.99%   +0.10%     
- Complexity      564      566       +2     
============================================
  Files            92       92              
  Lines          2770     2794      +24     
  Branches        268      271       +3     
============================================
+ Hits           2185     2207      +22     
- Misses          424      426       +2     
  Partials        161      161

Flag	Coverage Δ
integration	`78.99% <90.62%> (+0.10%)`	⬆️
unit	`49.42% <90.62%> (+0.36%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...transformer/PostgresSelectionQueryTransformer.java	`91.89% <90.62%> (-0.42%)`	⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

suresh-prakash · 2022-09-05T04:38:07Z

...nt-store/src/integrationTest/java/org/hypertrace/core/documentstore/DocStoreQueryV1Test.java

+            .setFilter(
+                RelationalExpression.of(
+                    IdentifierExpression.of("price"), LTE, ConstantExpression.of(10)))
+            .addAggregation(IdentifierExpression.of("item"))


Add group by on price as well, because it's a part of the selection?

suresh-prakash · 2022-09-05T04:40:51Z

...race/core/documentstore/postgres/query/v1/transformer/PostgresSelectionQueryTransformer.java

    List<SelectionSpec> finalSelectionSpecs =
        query.getSelections().stream()
-            .filter(selectionSpec -> selectionSpec.getExpression().accept(this))
+            .filter(
+                selectionSpec -> {


Could we keep the lambda definitions small by introducing private method to improve readability?

suresh-prakash · 2022-09-05T04:43:46Z

...race/core/documentstore/postgres/query/v1/transformer/PostgresSelectionQueryTransformer.java

-  public Boolean visit(AggregateExpression expression) {
-    return true;
-  }
+  private static class LocalSelectTypeAggregationExpressionSelector


If this is just returning the instance type, do need a separate visitor hierarchy? Could we just do using instance of?

I think, this is a more readable pattern.

suresh-prakash · 2022-09-05T04:44:06Z

...race/core/documentstore/postgres/query/v1/transformer/PostgresSelectionQueryTransformer.java

-  @Override
-  public Boolean visit(FunctionExpression expression) {
-    return false;
+  private static class LocalSelectTypeIdentifierExpressionSelector


suresh-prakash · 2022-09-05T04:44:20Z

...race/core/documentstore/postgres/query/v1/transformer/PostgresSelectionQueryTransformer.java

-  @Override
-  public Boolean visit(IdentifierExpression expression) {
-    return false;
+  private static class LocalGroupByIdentifierExpressionSelector


suresh-prakash · 2022-09-05T04:45:00Z

...c/test/java/org/hypertrace/core/documentstore/postgres/query/v1/PostgresQueryParserTest.java

+                RelationalExpression.of(
+                    IdentifierExpression.of("price"), LTE, ConstantExpression.of(10)))
+            .addAggregation(IdentifierExpression.of("item"))
+            .build();


Add price aggregation?

suresh-prakash · 2022-09-05T04:45:53Z

...c/test/java/org/hypertrace/core/documentstore/postgres/query/v1/PostgresQueryParserTest.java

+    String sql = postgresQueryParser.parse();
+
+    assertEquals(
+        "SELECT COUNT(DISTINCT document->>'quantity' ) AS \"qty_count\", "


It confuses to see where the price selection is lost!
Then, I remembered the transformation logic. Anyway, that'll come up.

Discussed offline.

github-actions · 2022-09-05T05:45:26Z

Unit Test Results

  16 files ±0   16 suites ±0 6s ⏱️ -1s
121 tests +1 121 ✔️ +1 0 💤 ±0 0 ❌ ±0
275 runs +3 275 ✔️ +3 0 💤 ±0 0 ❌ ±0

Results for commit 1306d65. ± Comparison against base commit 30c69aa.

kotharironak added 2 commits September 2, 2022 16:34

fix: adding group by checkers

63fd747

fix: handle group by more generically

4d5bd20

kotharironak requested review from avinashkolluru, skjindal93 and suresh-prakash as code owners September 5, 2022 04:34

This comment has been minimized.

Sign in to view

suresh-prakash reviewed Sep 5, 2022

View reviewed changes

addressed comments

5db4d5c

suresh-prakash previously approved these changes Sep 5, 2022

View reviewed changes

fixed the broken tests due to refactoring

d5e3360

kotharironak dismissed suresh-prakash’s stale review via d5e3360 September 5, 2022 05:39

This comment has been minimized.

Sign in to view

suresh-prakash approved these changes Sep 5, 2022

View reviewed changes

kotharironak merged commit 1306d65 into main Sep 5, 2022

kotharironak deleted the groupby-fix branch September 5, 2022 05:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: handle group_by identifier more generically #127

fix: handle group_by identifier more generically #127

kotharironak commented Sep 5, 2022

This comment has been minimized.

codecov bot commented Sep 5, 2022 •

edited

Loading

suresh-prakash Sep 5, 2022

suresh-prakash Sep 5, 2022

suresh-prakash Sep 5, 2022

kotharironak Sep 5, 2022

suresh-prakash Sep 5, 2022

suresh-prakash Sep 5, 2022

suresh-prakash Sep 5, 2022

suresh-prakash Sep 5, 2022

kotharironak Sep 5, 2022

This comment has been minimized.

github-actions bot commented Sep 5, 2022

fix: handle group_by identifier more generically #127

fix: handle group_by identifier more generically #127

Conversation

kotharironak commented Sep 5, 2022

This comment has been minimized.

codecov bot commented Sep 5, 2022 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

This comment has been minimized.

github-actions bot commented Sep 5, 2022

Unit Test Results

codecov bot commented Sep 5, 2022 •

edited

Loading