You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by GitBox <gi...@apache.org> on 2020/03/03 16:42:59 UTC
[GitHub] [drill] oleg-zinovev opened a new pull request #2009: DRILL-7622:
Compilation error when using HLL / TDigest with group by
oleg-zinovev opened a new pull request #2009: DRILL-7622: Compilation error when using HLL / TDigest with group by
URL: https://github.com/apache/drill/pull/2009
# [DRILL-7622](https://issues.apache.org/jira/browse/DRILL-7622): DRILL-7622: Compilation error when using HLL / TDigest with group by
## Description
Fixes code generation for workspace variables injected from OperationContext
## Testing
Appropriate unit test added (TestBugFixes#testDRILL7622)
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] vvysotskyi commented on a change in pull request #2009:
DRILL-7622: Compilation error when using HLL / TDigest with group by
Posted by GitBox <gi...@apache.org>.
vvysotskyi commented on a change in pull request #2009: DRILL-7622: Compilation error when using HLL / TDigest with group by
URL: https://github.com/apache/drill/pull/2009#discussion_r387160620
##########
File path: exec/java-exec/src/test/java/org/apache/drill/TestBugFixes.java
##########
@@ -316,4 +316,9 @@ public void testDRILL6318() throws Exception {
rows = testSql("SELECT FLATTEN(data) AS d FROM cp.`jsoninput/bug6318.json` LIMIT 3 OFFSET 5");
Assert.assertEquals(3, rows);
}
+
+ @Test
+ public void testDRILL7622() throws Exception {
+ test("SELECT tdigest(p.col_int) FROM cp.`parquet/alltypes_required.parquet` p GROUP BY p.col_flt");
Review comment:
Please use `testBuilder()` to verify the correctness of the result.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] vvysotskyi commented on a change in pull request #2009:
DRILL-7622: Compilation error when using HLL / TDigest with group by
Posted by GitBox <gi...@apache.org>.
vvysotskyi commented on a change in pull request #2009: DRILL-7622: Compilation error when using HLL / TDigest with group by
URL: https://github.com/apache/drill/pull/2009#discussion_r387160301
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/DrillAggFuncHolder.java
##########
@@ -159,7 +160,11 @@ public HoldingContainer renderEnd(ClassGenerator<?> classGenerator, HoldingConta
for (int i = 0; i < getWorkspaceVars().length; i++) {
if (getWorkspaceVars()[i].isInject()) {
workspaceJVars[i] = g.declareClassField("work", g.getModel()._ref(getWorkspaceVars()[i].getType()));
- g.getBlock(BlockType.SETUP).assign(workspaceJVars[i], g.getMappingSet().getIncoming().invoke("getContext").invoke("getManagedBuffer"));
+ g.getBlock(BlockType.SETUP).assign(
Review comment:
Could you please add checks similar to [`DrillFuncHolder`](https://github.com/apache/drill/blob/755529f3ac7ca77797f68b60e1d0713ad126e227/exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/DrillFuncHolder.java#L186) to ensure that the required variable should be injected, or even better - reformat code if possible to use common code for these two classes to inject variables in the same way?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] vvysotskyi commented on a change in pull request #2009:
DRILL-7622: Compilation error when using HLL / TDigest with group by
Posted by GitBox <gi...@apache.org>.
vvysotskyi commented on a change in pull request #2009: DRILL-7622: Compilation error when using HLL / TDigest with group by
URL: https://github.com/apache/drill/pull/2009#discussion_r387160301
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/DrillAggFuncHolder.java
##########
@@ -159,7 +160,11 @@ public HoldingContainer renderEnd(ClassGenerator<?> classGenerator, HoldingConta
for (int i = 0; i < getWorkspaceVars().length; i++) {
if (getWorkspaceVars()[i].isInject()) {
workspaceJVars[i] = g.declareClassField("work", g.getModel()._ref(getWorkspaceVars()[i].getType()));
- g.getBlock(BlockType.SETUP).assign(workspaceJVars[i], g.getMappingSet().getIncoming().invoke("getContext").invoke("getManagedBuffer"));
+ g.getBlock(BlockType.SETUP).assign(
Review comment:
Could you please add checks similar to [`DrillFuncHolder`](https://github.com/apache/drill/blob/755529f3ac7ca77797f68b60e1d0713ad126e227/exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/DrillFuncHolder.java#L185) to ensure that the required variable should be injected, or even better - reformat code if possible to use common code for these two classes to inject variables in the same way?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] asfgit closed pull request #2009: DRILL-7622: Compilation
error when using HLL / TDigest with group by
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #2009: DRILL-7622: Compilation error when using HLL / TDigest with group by
URL: https://github.com/apache/drill/pull/2009
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] vvysotskyi commented on a change in pull request #2009:
DRILL-7622: Compilation error when using HLL / TDigest with group by
Posted by GitBox <gi...@apache.org>.
vvysotskyi commented on a change in pull request #2009: DRILL-7622: Compilation error when using HLL / TDigest with group by
URL: https://github.com/apache/drill/pull/2009#discussion_r387163842
##########
File path: exec/java-exec/src/test/java/org/apache/drill/TestBugFixes.java
##########
@@ -316,4 +316,9 @@ public void testDRILL6318() throws Exception {
rows = testSql("SELECT FLATTEN(data) AS d FROM cp.`jsoninput/bug6318.json` LIMIT 3 OFFSET 5");
Assert.assertEquals(3, rows);
}
+
+ @Test
+ public void testDRILL7622() throws Exception {
Review comment:
Please move this test into `org.apache.drill.exec.fn.impl.TestAggregateFunctions` class and update its name to describe the issue it checks.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] vvysotskyi commented on a change in pull request #2009:
DRILL-7622: Compilation error when using HLL / TDigest with group by
Posted by GitBox <gi...@apache.org>.
vvysotskyi commented on a change in pull request #2009: DRILL-7622: Compilation error when using HLL / TDigest with group by
URL: https://github.com/apache/drill/pull/2009#discussion_r387160301
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/DrillAggFuncHolder.java
##########
@@ -159,7 +160,11 @@ public HoldingContainer renderEnd(ClassGenerator<?> classGenerator, HoldingConta
for (int i = 0; i < getWorkspaceVars().length; i++) {
if (getWorkspaceVars()[i].isInject()) {
workspaceJVars[i] = g.declareClassField("work", g.getModel()._ref(getWorkspaceVars()[i].getType()));
- g.getBlock(BlockType.SETUP).assign(workspaceJVars[i], g.getMappingSet().getIncoming().invoke("getContext").invoke("getManagedBuffer"));
+ g.getBlock(BlockType.SETUP).assign(
Review comment:
Could you please add checks similar to [`DrillFuncHolder`](https://github.com/apache/drill/blob/755529f3ac7ca77797f68b60e1d0713ad126e227/exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/DrillFuncHolder.java#L186) to ensure that the required variable should be injected, or even better - refactor code to use common code for these two classes to inject variables in the same way.
----------------------------------------------------------------
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
With regards,
Apache Git Services