You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by chunhui-shi <gi...@git.apache.org> on 2016/07/18 17:20:25 UTC
[GitHub] drill pull request #546: DRILL-4783: flatten operator should not throw excep...
GitHub user chunhui-shi opened a pull request:
https://github.com/apache/drill/pull/546
DRILL-4783: flatten operator should not throw exception if there is empty resultset
returned for underlying operator which is convert_from or other complex functions.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/chunhui-shi/drill DRILL-4783-flatten
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/drill/pull/546.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 #546
----
commit 88dc26dfeb7f47ded0bfdc4c7f89e6bc55520344
Author: chunhui-shi <cs...@maprtech.com>
Date: 2016-07-16T07:18:33Z
DRILL-4783: flatten operator handle empty resultset(and schema unknown due to 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 #546: DRILL-4783: flatten operator should not throw excep...
Posted by jinfengni <gi...@git.apache.org>.
Github user jinfengni commented on a diff in the pull request:
https://github.com/apache/drill/pull/546#discussion_r71825128
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/flatten/FlattenRecordBatch.java ---
@@ -267,6 +276,15 @@ private TransferPair getFlattenFieldTransferPair(FieldReference reference) {
if (flattenField instanceof RepeatedMapVector) {
tp = ((RepeatedMapVector)flattenField).getTransferPairToSingleMap(reference.getAsNamePart().getName(), oContext.getAllocator());
} else {
+ if ( !(flattenField instanceof RepeatedValueVector) ) {
+ if(incoming.getRecordCount() != 0) {
+ throw UserException.unsupportedError().message("Flatten does not support inputs of non-list values.").build(logger);
+ }
+ logger.error("Cannot cast {} to RepeatedValueVector", flattenField);
+ //when incoming recordCount is 0, don't throw exception since the type being seen here is not solid
+ ValueVector vv = new RepeatedMapVector(flattenField.getField(), oContext.getAllocator(), null);
+ tp = RepeatedValueVector.class.cast(vv).getTransferPair(reference.getAsNamePart().getName(), oContext.getAllocator());
+ }
final ValueVector vvIn = RepeatedValueVector.class.cast(flattenField).getDataVector();
--- End diff --
Looks like an else is missing here. Otherwise, the first new unit test (with empty input for flatten) will fail with ClassCastException.
---
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 issue #546: DRILL-4783: flatten operator should not throw exception if...
Posted by chunhui-shi <gi...@git.apache.org>.
Github user chunhui-shi commented on the issue:
https://github.com/apache/drill/pull/546
Thanks @jaltekruse for the review. I updated exception text and added test case following your suggestion.
---
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 #546: DRILL-4783: flatten operator should not throw excep...
Posted by jinfengni <gi...@git.apache.org>.
Github user jinfengni commented on a diff in the pull request:
https://github.com/apache/drill/pull/546#discussion_r71760001
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/flatten/FlattenRecordBatch.java ---
@@ -267,10 +276,21 @@ private TransferPair getFlattenFieldTransferPair(FieldReference reference) {
if (flattenField instanceof RepeatedMapVector) {
tp = ((RepeatedMapVector)flattenField).getTransferPairToSingleMap(reference.getAsNamePart().getName(), oContext.getAllocator());
} else {
- final ValueVector vvIn = RepeatedValueVector.class.cast(flattenField).getDataVector();
- // vvIn may be null because of fast schema return for repeated list vectors
- if (vvIn != null) {
- tp = vvIn.getTransferPair(reference.getAsNamePart().getName(), oContext.getAllocator());
+ try {
+ final ValueVector vvIn = RepeatedValueVector.class.cast(flattenField).getDataVector();
+ // vvIn may be null because of fast schema return for repeated list vectors
+ if (vvIn != null) {
+ tp = vvIn.getTransferPair(reference.getAsNamePart().getName(), oContext.getAllocator());
+ }
+ } catch(ClassCastException ex) {
--- End diff --
We can apply similar changes above here ( catch ClassCastException -> instancOf check), also, logger same message.
---
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 issue #546: DRILL-4783: flatten operator should not throw exception if...
Posted by jinfengni <gi...@git.apache.org>.
Github user jinfengni commented on the issue:
https://github.com/apache/drill/pull/546
LGTM.
---
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 issue #546: DRILL-4783: flatten operator should not throw exception if...
Posted by chunhui-shi <gi...@git.apache.org>.
Github user chunhui-shi commented on the issue:
https://github.com/apache/drill/pull/546
Update based on Jinfeng's comment. Jinfeng, could you check if I addressed your concern? 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 #546: DRILL-4783: flatten operator should not throw excep...
Posted by jinfengni <gi...@git.apache.org>.
Github user jinfengni commented on a diff in the pull request:
https://github.com/apache/drill/pull/546#discussion_r71630181
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/flatten/FlattenRecordBatch.java ---
@@ -129,11 +129,21 @@ private void setFlattenVector() {
try {
final TypedFieldId typedFieldId = incoming.getValueVectorId(popConfig.getColumn());
final MaterializedField field = incoming.getSchema().getColumn(typedFieldId.getFieldIds()[0]);
- final RepeatedValueVector vector = RepeatedValueVector.class.cast(incoming.getValueAccessorById(
- field.getValueClass(), typedFieldId.getFieldIds()).getValueVector());
+ RepeatedValueVector vector = null;
+ try {
+ vector = RepeatedValueVector.class.cast(incoming.getValueAccessorById(
+ field.getValueClass(), typedFieldId.getFieldIds()).getValueVector());
+ } catch(ClassCastException ex) {
--- End diff --
In stead of catch ClassCastException, you may check if the input value vector is an instance of RepeatedValueVector. Using try{} catch{} seems a bit over-kill for this case.
---
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 issue #546: DRILL-4783: flatten operator should not throw exception if...
Posted by jaltekruse <gi...@git.apache.org>.
Github user jaltekruse commented on the issue:
https://github.com/apache/drill/pull/546
One small last comment. The method testNoResult() doesn't actually verify that they query has no results, it just ignores whatever the query returns. I would recommend instead using the expectsEmptyResultSet() method on the test builder and marking this method deprecated, with a comment about the method on the builder as the preferred interface for such tests.
With that small fix I'm +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 #546: DRILL-4783: flatten operator should not throw excep...
Posted by jinfengni <gi...@git.apache.org>.
Github user jinfengni commented on a diff in the pull request:
https://github.com/apache/drill/pull/546#discussion_r71630375
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/flatten/FlattenRecordBatch.java ---
@@ -129,11 +129,21 @@ private void setFlattenVector() {
try {
final TypedFieldId typedFieldId = incoming.getValueVectorId(popConfig.getColumn());
final MaterializedField field = incoming.getSchema().getColumn(typedFieldId.getFieldIds()[0]);
- final RepeatedValueVector vector = RepeatedValueVector.class.cast(incoming.getValueAccessorById(
- field.getValueClass(), typedFieldId.getFieldIds()).getValueVector());
+ RepeatedValueVector vector = null;
+ try {
+ vector = RepeatedValueVector.class.cast(incoming.getValueAccessorById(
+ field.getValueClass(), typedFieldId.getFieldIds()).getValueVector());
+ } catch(ClassCastException ex) {
+ if(incoming.getRecordCount() != 0) {
+ throw UserException.unsupportedError(ex).message("Flatten does not support inputs of non-list values.").build(logger);
+ }
+ //when incoming recordCount is 0, don't throw exception since the type being seen here is not solid
+ logger.error("setFlattenVector cast failed and recordcount is 0, create empty vector anyway.");
+ vector = new RepeatedMapVector(field, oContext.getAllocator(), null);
+ }
flattener.setFlattenField(vector);
} catch (Exception ex) {
- throw UserException.unsupportedError(ex).message("Trying to flatten a non-repeated field.").build(logger);
+ throw UserException.unsupportedError(ex).message("Flatten does not support inputs of non-list values.").build(logger);
--- End diff --
UserException may be thrown twice? Line 138 & Line 146.
---
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 #546: DRILL-4783: flatten operator should not throw excep...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/drill/pull/546
---
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 issue #546: DRILL-4783: flatten operator should not throw exception if...
Posted by chunhui-shi <gi...@git.apache.org>.
Github user chunhui-shi commented on the issue:
https://github.com/apache/drill/pull/546
@jaltekruse Hi Jason, could you help to review this change? 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 issue #546: DRILL-4783: flatten operator should not throw exception if...
Posted by jinfengni <gi...@git.apache.org>.
Github user jinfengni commented on the issue:
https://github.com/apache/drill/pull/546
Thanks, @jaltekruse . I'll make the change you recommended, and squashed into Chunhui's 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 #546: DRILL-4783: flatten operator should not throw excep...
Posted by chunhui-shi <gi...@git.apache.org>.
Github user chunhui-shi commented on a diff in the pull request:
https://github.com/apache/drill/pull/546#discussion_r71471032
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/flatten/FlattenRecordBatch.java ---
@@ -129,8 +129,14 @@ private void setFlattenVector() {
try {
final TypedFieldId typedFieldId = incoming.getValueVectorId(popConfig.getColumn());
final MaterializedField field = incoming.getSchema().getColumn(typedFieldId.getFieldIds()[0]);
- final RepeatedValueVector vector = RepeatedValueVector.class.cast(incoming.getValueAccessorById(
- field.getValueClass(), typedFieldId.getFieldIds()).getValueVector());
+ RepeatedValueVector vector = null;
+ try {
+ vector = RepeatedValueVector.class.cast(incoming.getValueAccessorById(
+ field.getValueClass(), typedFieldId.getFieldIds()).getValueVector());
+ } catch(ClassCastException ex) {
+ //if cast fail
--- End diff --
Thanks Jinfeng for the review. The fix will return empty result if it is flatten(100).
Since this fix is to address the issue when there is no incoming record, flatten above convert_from should not throw exception. Meanwhile it is better to keep other behavior unchanged unless we have strong reason to change it. Based on this understanding, I am updating the change to still throw the same cast exception if flatten is not getting a RepeatedValueVector to work on.
---
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 #546: DRILL-4783: flatten operator should not throw excep...
Posted by jinfengni <gi...@git.apache.org>.
Github user jinfengni commented on a diff in the pull request:
https://github.com/apache/drill/pull/546#discussion_r71368874
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/flatten/FlattenRecordBatch.java ---
@@ -129,8 +129,14 @@ private void setFlattenVector() {
try {
final TypedFieldId typedFieldId = incoming.getValueVectorId(popConfig.getColumn());
final MaterializedField field = incoming.getSchema().getColumn(typedFieldId.getFieldIds()[0]);
- final RepeatedValueVector vector = RepeatedValueVector.class.cast(incoming.getValueAccessorById(
- field.getValueClass(), typedFieldId.getFieldIds()).getValueVector());
+ RepeatedValueVector vector = null;
+ try {
+ vector = RepeatedValueVector.class.cast(incoming.getValueAccessorById(
+ field.getValueClass(), typedFieldId.getFieldIds()).getValueVector());
+ } catch(ClassCastException ex) {
+ //if cast fail
--- End diff --
If the input to flatten happens to be a scalar value, i.e (int 100), what's the desired result? The previous behavior is to hit ClassCastException, which might need improve the error message. With your change, flatten(100) will return a single value of (100), or just an empty repeated map?
---
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 #546: DRILL-4783: flatten operator should not throw excep...
Posted by jinfengni <gi...@git.apache.org>.
Github user jinfengni commented on a diff in the pull request:
https://github.com/apache/drill/pull/546#discussion_r71629104
--- Diff: exec/java-exec/src/test/java/org/apache/drill/exec/nested/TestFastComplexSchema.java ---
@@ -80,4 +80,33 @@ public void test4() throws Exception {
" AND r.r_regionkey = 4)) t \n" +
"ORDER BY t.f.name");
}
+
+ @Test //DRILL-4783 when resultset is empty, don't throw exception.
+ public void test5() throws Exception {
+ test("alter session set `planner.enable_hashjoin` = false");
--- End diff --
Any reason why you need set these two options?
---
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 issue #546: DRILL-4783: flatten operator should not throw exception if...
Posted by jinfengni <gi...@git.apache.org>.
Github user jinfengni commented on the issue:
https://github.com/apache/drill/pull/546
I picked the latest commit, and am running the regression. Will merge to master if everything runs fine.
---
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 issue #546: DRILL-4783: flatten operator should not throw exception if...
Posted by jinfengni <gi...@git.apache.org>.
Github user jinfengni commented on the issue:
https://github.com/apache/drill/pull/546
+1
Overall LGTM.
@jaltekruse , do you have any further comment? thx.
---
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 #546: DRILL-4783: flatten operator should not throw excep...
Posted by chunhui-shi <gi...@git.apache.org>.
Github user chunhui-shi commented on a diff in the pull request:
https://github.com/apache/drill/pull/546#discussion_r71634838
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/flatten/FlattenRecordBatch.java ---
@@ -129,11 +129,21 @@ private void setFlattenVector() {
try {
final TypedFieldId typedFieldId = incoming.getValueVectorId(popConfig.getColumn());
final MaterializedField field = incoming.getSchema().getColumn(typedFieldId.getFieldIds()[0]);
- final RepeatedValueVector vector = RepeatedValueVector.class.cast(incoming.getValueAccessorById(
- field.getValueClass(), typedFieldId.getFieldIds()).getValueVector());
+ RepeatedValueVector vector = null;
+ try {
+ vector = RepeatedValueVector.class.cast(incoming.getValueAccessorById(
+ field.getValueClass(), typedFieldId.getFieldIds()).getValueVector());
+ } catch(ClassCastException ex) {
+ if(incoming.getRecordCount() != 0) {
+ throw UserException.unsupportedError(ex).message("Flatten does not support inputs of non-list values.").build(logger);
+ }
+ //when incoming recordCount is 0, don't throw exception since the type being seen here is not solid
+ logger.error("setFlattenVector cast failed and recordcount is 0, create empty vector anyway.");
+ vector = new RepeatedMapVector(field, oContext.getAllocator(), null);
+ }
flattener.setFlattenField(vector);
} catch (Exception ex) {
- throw UserException.unsupportedError(ex).message("Trying to flatten a non-repeated field.").build(logger);
+ throw UserException.unsupportedError(ex).message("Flatten does not support inputs of non-list values.").build(logger);
--- End diff --
The two catch blocks are parallel. The UserException in line 138 wont be caught and thrown again in line 146.
---
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 issue #546: DRILL-4783: flatten operator should not throw exception if...
Posted by jaltekruse <gi...@git.apache.org>.
Github user jaltekruse commented on the issue:
https://github.com/apache/drill/pull/546
The original behavior was not by design, it was a bug. I would just detect the case where a scalar is being flattened and throw an appropriate user facing message like "Flatten does not support inputs of non-list values".
The tests you have included here don't check their results. I would also suggest adding a test for the new negative case, checking for the appropriate message.
If you want to more precisely control the input to your test, I would recommend taking a look at the operator unit test system I checked in and demonstrated at the hangout last month. This will allow you to write more targeted tests for this case. Feel free to ask questions if the usage is not clear from the examples in this class.
https://github.com/apache/drill/blob/master/exec/java-exec/src/test/java/org/apache/drill/exec/physical/unit/BasicPhysicalOpUnitTest.java
---
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.
---