You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2020/08/12 08:16:44 UTC
[GitHub] [incubator-pinot] fx19880617 opened a new pull request #5851: Support multi-value non-dictionary group by
fx19880617 opened a new pull request #5851:
URL: https://github.com/apache/incubator-pinot/pull/5851
## Description
Support multi-value non-dictionary group by
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #5851: Support multi-value non-dictionary group by
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #5851:
URL: https://github.com/apache/incubator-pinot/pull/5851#discussion_r469439805
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/groupby/NoDictionaryMultiColumnGroupKeyGenerator.java
##########
@@ -177,6 +305,29 @@ private int getGroupIdForKey(FixedIntArray keyList) {
return groupId;
}
+ /**
+ * Helper method to get or create a list of group-id for a list of group key.
+ *
+ * @param keysList Group keys, that is a list of list of objects to be grouped
+ * @return Group ids
+ */
+ private int[] getGroupIdsForKey(int[][] keysList) {
+ List<Integer> groupIds = new ArrayList<>();
+ getGroupIdsForKeyHelper(keysList, new int[keysList.length], 0, groupIds);
+ return groupIds.stream().mapToInt(i->i).toArray();
Review comment:
Avoid using stream in query engine for performance concern.
To avoid boxing/unboxing, you may use fastutil `IntArrayList` and `toIntArray()` to get the primitive array.
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/groupby/NoDictionaryMultiColumnGroupKeyGenerator.java
##########
@@ -177,6 +305,29 @@ private int getGroupIdForKey(FixedIntArray keyList) {
return groupId;
}
+ /**
+ * Helper method to get or create a list of group-id for a list of group key.
+ *
+ * @param keysList Group keys, that is a list of list of objects to be grouped
+ * @return Group ids
+ */
+ private int[] getGroupIdsForKey(int[][] keysList) {
+ List<Integer> groupIds = new ArrayList<>();
+ getGroupIdsForKeyHelper(keysList, new int[keysList.length], 0, groupIds);
+ return groupIds.stream().mapToInt(i->i).toArray();
+ }
+
+ private void getGroupIdsForKeyHelper(int[][] keysList, int[] groupKeyIds, int level, List<Integer> groupIds) {
+ if (level == keysList.length) {
+ groupIds.add(getGroupIdForKey(new FixedIntArray(groupKeyIds)));
+ return;
+ }
+ for (int i = 0; i < keysList[level].length; i++) {
Review comment:
(nit) Store `keysList[level].length` as a local variable
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/groupby/NoDictionaryMultiColumnGroupKeyGenerator.java
##########
@@ -177,6 +305,29 @@ private int getGroupIdForKey(FixedIntArray keyList) {
return groupId;
}
+ /**
+ * Helper method to get or create a list of group-id for a list of group key.
+ *
+ * @param keysList Group keys, that is a list of list of objects to be grouped
+ * @return Group ids
+ */
+ private int[] getGroupIdsForKey(int[][] keysList) {
Review comment:
(nit)
```suggestion
private int[] getGroupIdsForKeys(int[][] keysList) {
```
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/groupby/NoDictionaryMultiColumnGroupKeyGenerator.java
##########
@@ -177,6 +305,29 @@ private int getGroupIdForKey(FixedIntArray keyList) {
return groupId;
}
+ /**
+ * Helper method to get or create a list of group-id for a list of group key.
+ *
+ * @param keysList Group keys, that is a list of list of objects to be grouped
+ * @return Group ids
+ */
+ private int[] getGroupIdsForKey(int[][] keysList) {
+ List<Integer> groupIds = new ArrayList<>();
+ getGroupIdsForKeyHelper(keysList, new int[keysList.length], 0, groupIds);
+ return groupIds.stream().mapToInt(i->i).toArray();
+ }
+
+ private void getGroupIdsForKeyHelper(int[][] keysList, int[] groupKeyIds, int level, List<Integer> groupIds) {
+ if (level == keysList.length) {
+ groupIds.add(getGroupIdForKey(new FixedIntArray(groupKeyIds)));
Review comment:
(Critical) Need to make a copy of `groupKeyIds`, because it can be inserted into the map
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] fx19880617 merged pull request #5851: Support multi-value non-dictionary group by
Posted by GitBox <gi...@apache.org>.
fx19880617 merged pull request #5851:
URL: https://github.com/apache/incubator-pinot/pull/5851
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] Jackie-Jiang commented on pull request #5851: Support multi-value non-dictionary group by
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on pull request #5851:
URL: https://github.com/apache/incubator-pinot/pull/5851#issuecomment-673128645
I think H2 does not support MV columns, so you might need to add the test into `MultiValueQueriesTest`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org