fix : group by on functional expressions in mongo #200

RishabhB99 · 2024-06-28T06:13:53Z

Description

Bug fix for grouping on functional expressions

Testing

Added a test for the same scenario

Checklist:

My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
Any dependent changes have been merged and published in downstream modules

Documentation

Make sure that you have documented corresponding changes in this repository or hypertrace docs repo if required.

suresh-prakash · 2024-06-28T11:46:48Z

document-store/src/integrationTest/resources/query/function_expression_group_by_response.json

+    "function": 3.0,
+    "functionCount": 2
+  }
+]


Nit: Empty line at the end.

suresh-prakash · 2024-06-28T11:48:26Z

document-store/src/main/java/org/hypertrace/core/documentstore/mongo/MongoUtils.java

@@ -145,6 +150,20 @@ public static ReturnDocument getReturnDocument(final ReturnDocumentType returnDo
                    String.format("Unhandled return document type: %s", returnDocumentType)));
  }

+  public static boolean isFunctionExpressionSelectionWithGroupBy(


Utils is only for very specific and common things. Can we move this method out of the utils?

this is used at 2 places at MongoGroupTypeExpressionParser and MongoSelectTypeExpressionParser
so we'll need to define it at some common place, right?

It makes sense to put them in a common place, but doesn't feel like "Utils" is the right place. Either we could have a shared class (non-static) or have the definition in one place and access it in another.

suresh-prakash · 2024-06-28T11:49:02Z

document-store/src/main/java/org/hypertrace/core/documentstore/mongo/MongoUtils.java

+      final SelectionSpec selectionSpec, final List<String> groupByAliases) {
+    return selectionSpec.getAlias() != null
+        && groupByAliases.contains(selectionSpec.getAlias())
+        && selectionSpec.getExpression().getClass().equals(FunctionExpression.class);


Please avoid checking class instances like this. Refer classes like IdentifierChecker, etc.

suresh-prakash · 2024-06-28T11:49:54Z

document-store/src/main/java/org/hypertrace/core/documentstore/mongo/MongoUtils.java

+  public static List<String> getGroupByAliases(final List<GroupTypeExpression> expressions) {
+    return expressions.stream()
+        .filter(expression -> expression.getClass().equals(IdentifierExpression.class))
+        .map(expression -> ((IdentifierExpression) expression).getName())


Could you avoid explicit instance of/class equality checks and casting by leveraging the visitor pattern?

suresh-prakash · 2024-06-28T11:52:02Z

...ava/org/hypertrace/core/documentstore/mongo/query/parser/MongoGroupTypeExpressionParser.java

+          functionExpressionSelectionWithGroupBys.stream()
+              .map(spec -> MongoGroupTypeExpressionParser.parse(parser, spec))
+              .reduce(
+                  new LinkedHashMap<>(),


Any reason for a linked hashmap? Is the order important here?

no reason, i just copy pasted the reduce used below
don't think the order is important

suresh-prakash · 2024-06-28T11:52:59Z

...ava/org/hypertrace/core/documentstore/mongo/query/parser/MongoGroupTypeExpressionParser.java

@@ -99,4 +128,15 @@ private Map<String, Object> parse(final GroupTypeExpression expression) {
    MongoGroupTypeExpressionParser parser = new MongoGroupTypeExpressionParser();
    return expression.accept(parser);
  }
+
+  private static List<SelectionSpec> getFunctionExpressionSelectionWithGroupBys(


Where is this getting used?

document-store/document-store/src/main/java/org/hypertrace/core/documentstore/mongo/query/parser/MongoGroupTypeExpressionParser.java

Line 56 in 93fdf17

getFunctionExpressionSelectionWithGroupBys(selectionSpecs, expressions);

suresh-prakash · 2024-07-02T09:13:29Z

document-store/src/main/java/org/hypertrace/core/documentstore/parser/GroupByAliasGetter.java

+
+  @Override
+  public String visit(FunctionExpression expression) {
+    return null;


Better to throw exception instead of null.
Or, alternatively return optional of string to avoid NullPointerExceptions.

suresh-prakash · 2024-07-02T11:05:11Z

...ava/org/hypertrace/core/documentstore/mongo/query/parser/MongoGroupTypeExpressionParser.java

+        .filter(Optional::isPresent)
+        .map(Optional::get)


Nit: Can also be simplified to.

Suggested change

.filter(Optional::isPresent)

.map(Optional::get)

.flatMap(Optional::stream)

codecov · 2024-07-02T12:22:42Z

Codecov Report

Attention: Patch coverage is 97.56098% with 1 line in your changes missing coverage. Please review.

Project coverage is 79.65%. Comparing base (43b12e8) to head (948d214).

Files	Patch %	Lines
.../core/documentstore/parser/GroupByAliasGetter.java	66.66%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main     #200      +/-   ##
============================================
+ Coverage     79.45%   79.65%   +0.20%     
- Complexity     1003     1013      +10     
============================================
  Files           193      194       +1     
  Lines          4769     4807      +38     
  Branches        399      402       +3     
============================================
+ Hits           3789     3829      +40     
+ Misses          698      696       -2     
  Partials        282      282

Flag	Coverage Δ
integration	`79.65% <97.56%> (+0.20%)`	⬆️
unit	`57.18% <58.53%> (+<0.01%)`	⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

fix : group by on functional expressions in mongo

93fdf17

RishabhB99 requested review from avinashkolluru, kotharironak, skjindal93 and suresh-prakash as code owners June 28, 2024 06:13

suresh-prakash reviewed Jun 28, 2024

View reviewed changes

addressed pr comments

46737e1

RishabhB99 requested a review from suresh-prakash July 2, 2024 06:04

suresh-prakash reviewed Jul 2, 2024

View reviewed changes

addressed pr comment

948d214

RishabhB99 requested a review from suresh-prakash July 2, 2024 10:25

suresh-prakash approved these changes Jul 2, 2024

View reviewed changes

suresh-prakash merged commit c7ea09e into hypertrace:main Jul 2, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix : group by on functional expressions in mongo #200

fix : group by on functional expressions in mongo #200

RishabhB99 commented Jun 28, 2024 •

edited

Loading

suresh-prakash Jun 28, 2024

suresh-prakash Jun 28, 2024

RishabhB99 Jun 28, 2024

suresh-prakash Jun 28, 2024

suresh-prakash Jun 28, 2024

suresh-prakash Jun 28, 2024

suresh-prakash Jun 28, 2024

RishabhB99 Jun 28, 2024

suresh-prakash Jun 28, 2024

RishabhB99 Jun 28, 2024

suresh-prakash Jul 2, 2024

suresh-prakash Jul 2, 2024

codecov bot commented Jul 2, 2024 •

edited

Loading

	.filter(Optional::isPresent)
	.map(Optional::get)
	.flatMap(Optional::stream)

fix : group by on functional expressions in mongo #200

fix : group by on functional expressions in mongo #200

Conversation

RishabhB99 commented Jun 28, 2024 • edited Loading

Description

Testing

Checklist:

Documentation

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 2, 2024 • edited Loading

Codecov Report

RishabhB99 commented Jun 28, 2024 •

edited

Loading

codecov bot commented Jul 2, 2024 •

edited

Loading