You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by hsuanyi <gi...@git.apache.org> on 2016/03/05 00:28:52 UTC
[GitHub] drill pull request: DRILL-4476: Allow UnionAllRecordBatch to manag...
GitHub user hsuanyi opened a pull request:
https://github.com/apache/drill/pull/407
DRILL-4476: Allow UnionAllRecordBatch to manager situations where lef…
…t input side or both sides come(s) from empty source(s).
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/hsuanyi/incubator-drill DRILL-4476
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/drill/pull/407.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #407
----
commit 5a384a14f23827aee8b20a8ac6529b33bc66ee2e
Author: Hsuan-Yi Chu <hs...@usc.edu>
Date: 2016-03-04T21:50:02Z
DRILL-4476: Allow UnionAllRecordBatch to manager situations where left input side or both sides come(s) from empty source(s).
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4476: Allow UnionAllRecordBatch to manag...
Posted by amansinha100 <gi...@git.apache.org>.
Github user amansinha100 commented on a diff in the pull request:
https://github.com/apache/drill/pull/407#discussion_r55779800
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/union/UnionAllRecordBatch.java ---
@@ -294,13 +313,41 @@ public UnionAllInput(UnionAllRecordBatch unionAllRecordBatch, RecordBatch left,
rightSide = new OneSideInput(right);
}
+ private boolean isBothSideEmpty() {
+ return leftIsFinish && rightIsFinish;
+ }
+
public IterOutcome nextBatch() throws SchemaChangeException {
if(upstream == RecordBatch.IterOutcome.NOT_YET) {
IterOutcome iterLeft = leftSide.nextBatch();
switch(iterLeft) {
case OK_NEW_SCHEMA:
- break;
+ whileLoop:
+ while(leftSide.getRecordBatch().getRecordCount() == 0) {
+ iterLeft = leftSide.nextBatch();
+
+ switch(iterLeft) {
+ case STOP:
+ case OUT_OF_MEMORY:
+ return iterLeft;
+
+ case NONE:
--- End diff --
Could you add comments here about why we need to keep reading after getting OK_NEW_SCHEMA. Also, do any of the union-all tests (either unit or functional) exercise this code ? i.e where the left input has 2 or more empty files ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4476: Allow UnionAllRecordBatch to manag...
Posted by amansinha100 <gi...@git.apache.org>.
Github user amansinha100 commented on a diff in the pull request:
https://github.com/apache/drill/pull/407#discussion_r55557460
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/union/UnionAllRecordBatch.java ---
@@ -162,6 +162,25 @@ private IterOutcome doWork() throws ClassTransformationException, IOException, S
allocationVectors = Lists.newArrayList();
transfers.clear();
+ // If both sides of Union-All are empty
+ if(unionAllInput.isBothSideEmpty()) {
+ for(int i = 0; i < outputFields.size(); ++i) {
--- End diff --
If both sides are empty, wouldn't outputFields.size() be 0 ? Or is outputFields populated from a previous batch, which implies both sides may not have been empty. Can you clarify ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4476: Allow UnionAllRecordBatch to manag...
Posted by amansinha100 <gi...@git.apache.org>.
Github user amansinha100 commented on a diff in the pull request:
https://github.com/apache/drill/pull/407#discussion_r55561188
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/union/UnionAllRecordBatch.java ---
@@ -294,13 +313,41 @@ public UnionAllInput(UnionAllRecordBatch unionAllRecordBatch, RecordBatch left,
rightSide = new OneSideInput(right);
}
+ private boolean isBothSideEmpty() {
+ return leftIsFinish && rightIsFinish;
+ }
+
public IterOutcome nextBatch() throws SchemaChangeException {
if(upstream == RecordBatch.IterOutcome.NOT_YET) {
IterOutcome iterLeft = leftSide.nextBatch();
switch(iterLeft) {
case OK_NEW_SCHEMA:
- break;
+ whileLoop:
+ while(leftSide.getRecordBatch().getRecordCount() == 0) {
+ iterLeft = leftSide.nextBatch();
+
+ switch(iterLeft) {
+ case STOP:
+ case OUT_OF_MEMORY:
+ return iterLeft;
+
+ case NONE:
--- End diff --
Is the NONE state not reached outside this while loop ? It wasn't clear why we need the inner while loop for the left input considering that you don't do that for the right input.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4476: Allow UnionAllRecordBatch to manag...
Posted by hsuanyi <gi...@git.apache.org>.
Github user hsuanyi commented on a diff in the pull request:
https://github.com/apache/drill/pull/407#discussion_r55595195
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/union/UnionAllRecordBatch.java ---
@@ -162,6 +162,25 @@ private IterOutcome doWork() throws ClassTransformationException, IOException, S
allocationVectors = Lists.newArrayList();
transfers.clear();
+ // If both sides of Union-All are empty
+ if(unionAllInput.isBothSideEmpty()) {
+ for(int i = 0; i < outputFields.size(); ++i) {
--- End diff --
For example, you could do:
select a, b, c from empty union all select a, b, c from empty
Then, the output should be (a, b, c) as column names.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4476: Allow UnionAllRecordBatch to manag...
Posted by hsuanyi <gi...@git.apache.org>.
Github user hsuanyi commented on the pull request:
https://github.com/apache/drill/pull/407#issuecomment-192523111
@amansinha100 can you help review this?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4476: Allow UnionAllRecordBatch to manag...
Posted by hsuanyi <gi...@git.apache.org>.
Github user hsuanyi commented on a diff in the pull request:
https://github.com/apache/drill/pull/407#discussion_r55596051
--- Diff: exec/java-exec/src/test/java/org/apache/drill/TestUnionAll.java ---
@@ -527,6 +532,52 @@ public void testUnionAllRightEmptyJson() throws Exception {
.build().run();
}
+ @Test
+ public void testUnionAllLeftEmptyJson() throws Exception {
+ final String rootEmpty = FileUtils.getResourceAsFile("/project/pushdown/empty.json").toURI().toString();
+ final String rootSimple = FileUtils.getResourceAsFile("/store/json/booleanData.json").toURI().toString();
+
+ final String queryRightEmpty = String.format(
--- End diff --
addressed
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4476: Allow UnionAllRecordBatch to manag...
Posted by amansinha100 <gi...@git.apache.org>.
Github user amansinha100 commented on the pull request:
https://github.com/apache/drill/pull/407#issuecomment-195138369
Updated changes LGTM. +1
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4476: Allow UnionAllRecordBatch to manag...
Posted by amansinha100 <gi...@git.apache.org>.
Github user amansinha100 commented on a diff in the pull request:
https://github.com/apache/drill/pull/407#discussion_r55561579
--- Diff: exec/java-exec/src/test/java/org/apache/drill/TestUnionAll.java ---
@@ -527,6 +532,52 @@ public void testUnionAllRightEmptyJson() throws Exception {
.build().run();
}
+ @Test
+ public void testUnionAllLeftEmptyJson() throws Exception {
--- End diff --
Are there any union-all tests with WHERE 1 = 0 filters on the left and/or right inputs ? If not, could you add such tests ? We should distinguish between a truly empty input vs. one that occurs as a result of a False filter.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4476: Allow UnionAllRecordBatch to manag...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/drill/pull/407
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4476: Allow UnionAllRecordBatch to manag...
Posted by hsuanyi <gi...@git.apache.org>.
Github user hsuanyi commented on a diff in the pull request:
https://github.com/apache/drill/pull/407#discussion_r55597640
--- Diff: exec/java-exec/src/test/java/org/apache/drill/TestUnionAll.java ---
@@ -527,6 +532,52 @@ public void testUnionAllRightEmptyJson() throws Exception {
.build().run();
}
+ @Test
+ public void testUnionAllLeftEmptyJson() throws Exception {
--- End diff --
Added a few for both union-all and union-distinct
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4476: Allow UnionAllRecordBatch to manag...
Posted by amansinha100 <gi...@git.apache.org>.
Github user amansinha100 commented on a diff in the pull request:
https://github.com/apache/drill/pull/407#discussion_r55749381
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/union/UnionAllRecordBatch.java ---
@@ -162,6 +162,25 @@ private IterOutcome doWork() throws ClassTransformationException, IOException, S
allocationVectors = Lists.newArrayList();
transfers.clear();
+ // If both sides of Union-All are empty
+ if(unionAllInput.isBothSideEmpty()) {
+ for(int i = 0; i < outputFields.size(); ++i) {
--- End diff --
Ok, I suppose we are not supporting the SELECT * case, so the output fields only depends on what is specified in the SELECT list, not from the table.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4476: Allow UnionAllRecordBatch to manag...
Posted by hsuanyi <gi...@git.apache.org>.
Github user hsuanyi commented on a diff in the pull request:
https://github.com/apache/drill/pull/407#discussion_r55605493
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/union/UnionAllRecordBatch.java ---
@@ -491,6 +556,25 @@ private void inferOutputFieldsFromLeftSide() {
}
}
+ private void inferOutputFieldsFromRightSide() {
--- End diff --
Inferencing happens only once when Drill receives the very first batches from left and right. (Schema-change is not yet supported for Union-All).
Let me summarize the inference in four different situations:
First of all, the field names are always determined by the left side (even when the left side is from empty file, we have the column names. Please see the comment above.)
1. Left: non-empty; Right: non-empty=> types determined by both sides with implicit casting involved
2. Left: empty; Right: non-empty=> type from the right
3. Left: non-empty; Right: empty=> types from the left
4. Left: empty; Right: empty=> types are nullable integer
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4476: Allow UnionAllRecordBatch to manag...
Posted by hsuanyi <gi...@git.apache.org>.
Github user hsuanyi commented on a diff in the pull request:
https://github.com/apache/drill/pull/407#discussion_r55597867
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/union/UnionAllRecordBatch.java ---
@@ -294,13 +313,41 @@ public UnionAllInput(UnionAllRecordBatch unionAllRecordBatch, RecordBatch left,
rightSide = new OneSideInput(right);
}
+ private boolean isBothSideEmpty() {
+ return leftIsFinish && rightIsFinish;
+ }
+
public IterOutcome nextBatch() throws SchemaChangeException {
if(upstream == RecordBatch.IterOutcome.NOT_YET) {
IterOutcome iterLeft = leftSide.nextBatch();
switch(iterLeft) {
case OK_NEW_SCHEMA:
- break;
+ whileLoop:
+ while(leftSide.getRecordBatch().getRecordCount() == 0) {
+ iterLeft = leftSide.nextBatch();
+
+ switch(iterLeft) {
+ case STOP:
+ case OUT_OF_MEMORY:
+ return iterLeft;
+
+ case NONE:
--- End diff --
The while is for the case where the first few batches are empty and then union-all receive NONE.
So we need to replicate this logic to right side too.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4476: Allow UnionAllRecordBatch to manag...
Posted by amansinha100 <gi...@git.apache.org>.
Github user amansinha100 commented on a diff in the pull request:
https://github.com/apache/drill/pull/407#discussion_r55559565
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/union/UnionAllRecordBatch.java ---
@@ -491,6 +556,25 @@ private void inferOutputFieldsFromLeftSide() {
}
}
+ private void inferOutputFieldsFromRightSide() {
--- End diff --
It seems like the inferencing is repeated a few times. You could have a single function like inferOutputFieldNameAndType(outputFieldNames, batch). The field type is always inferred from the supplied batch. For the output names, if the outputFieldNames is null, then use the output field names as well as type from that batch (this would be the case for left input). If not null, then use the field names. Would that work ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4476: Allow UnionAllRecordBatch to manag...
Posted by hsuanyi <gi...@git.apache.org>.
Github user hsuanyi commented on the pull request:
https://github.com/apache/drill/pull/407#issuecomment-195794797
Comments were added to the code; Also passed all the pre commit test. Can you help commit this patch ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4476: Allow UnionAllRecordBatch to manag...
Posted by hsuanyi <gi...@git.apache.org>.
Github user hsuanyi commented on the pull request:
https://github.com/apache/drill/pull/407#issuecomment-194598924
@amansinha100 I consolidated the two logic into one:
https://github.com/hsuanyi/incubator-drill/blob/DRILL-4476/exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/union/UnionAllRecordBatch.java#L597
Can you help review again? Thanks.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-4476: Allow UnionAllRecordBatch to manag...
Posted by amansinha100 <gi...@git.apache.org>.
Github user amansinha100 commented on a diff in the pull request:
https://github.com/apache/drill/pull/407#discussion_r55558287
--- Diff: exec/java-exec/src/test/java/org/apache/drill/TestUnionAll.java ---
@@ -527,6 +532,52 @@ public void testUnionAllRightEmptyJson() throws Exception {
.build().run();
}
+ @Test
+ public void testUnionAllLeftEmptyJson() throws Exception {
+ final String rootEmpty = FileUtils.getResourceAsFile("/project/pushdown/empty.json").toURI().toString();
+ final String rootSimple = FileUtils.getResourceAsFile("/store/json/booleanData.json").toURI().toString();
+
+ final String queryRightEmpty = String.format(
--- End diff --
The name suggests right is empty but the query has left empty...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---