You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by jinfengni <gi...@git.apache.org> on 2017/03/27 23:21:35 UTC
[GitHub] drill pull request #801: DRILL-5378: Put more information for schema change ...
GitHub user jinfengni opened a pull request:
https://github.com/apache/drill/pull/801
DRILL-5378: Put more information for schema change exception in hash \u2026
\u2026join, hash agg and sort operator.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/jinfengni/incubator-drill DRILL-5378
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/drill/pull/801.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 #801
----
commit e29909a08146c5c766590c07015b5ba9137a8dee
Author: Jinfeng Ni <jn...@apache.org>
Date: 2017-03-22T22:28:22Z
DRILL-5378: Put more information for schema change exception in hash join, hash agg and sort operator.
----
---
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 #801: DRILL-5378: Put more information for schema change ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/drill/pull/801
---
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 #801: DRILL-5378: Put more information for schema change excepti...
Posted by jinfengni <gi...@git.apache.org>.
Github user jinfengni commented on the issue:
https://github.com/apache/drill/pull/801
Addressed review comments. Pls let me know if you have any further comments. 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 issue #801: DRILL-5378: Put more information for schema change excepti...
Posted by gparai <gi...@git.apache.org>.
Github user gparai commented on the issue:
https://github.com/apache/drill/pull/801
Would it be a good idea to add tests for the same? Otherwise, 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 #801: DRILL-5378: Put more information for schema change excepti...
Posted by amansinha100 <gi...@git.apache.org>.
Github user amansinha100 commented on the issue:
https://github.com/apache/drill/pull/801
+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 issue #801: DRILL-5378: Put more information for schema change excepti...
Posted by jinfengni <gi...@git.apache.org>.
Github user jinfengni commented on the issue:
https://github.com/apache/drill/pull/801
If we change the flow to handle schema change, it makes senses to add new tests. Here, we only changes the error message ; not how we handle scheme change. I'm not inclined to add new tests just to verify the error message, since error message might involve over time.
---
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 #801: DRILL-5378: Put more information for schema change ...
Posted by gparai <gi...@git.apache.org>.
Github user gparai commented on a diff in the pull request:
https://github.com/apache/drill/pull/801#discussion_r108493803
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/aggregate/HashAggTemplate.java ---
@@ -324,9 +328,9 @@ public AggOutcome doWork() {
logger.debug("Received new schema. Batch has {} records.", incoming.getRecordCount());
}
// newSchema = true;
- this.cleanup();
// TODO: new schema case needs to be handled appropriately
- return AggOutcome.UPDATE_AGGREGATOR;
+ this.cleanup();
+ throw SchemaChangeException.schemChanged("Hash aggregate does not support schema changes", schema, incoming.getSchema());
--- End diff --
Please correct to the renamed function here and elsewhere.
---
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 #801: DRILL-5378: Put more information for schema change ...
Posted by gparai <gi...@git.apache.org>.
Github user gparai commented on a diff in the pull request:
https://github.com/apache/drill/pull/801#discussion_r108492668
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/exception/SchemaChangeException.java ---
@@ -49,4 +50,16 @@ public SchemaChangeException(String message, Object...objects){
public SchemaChangeException(String message, Throwable cause, Object...objects){
super(String.format(message, objects), cause);
}
+
+ public static SchemaChangeException schemChanged(String message, BatchSchema priorSchema, BatchSchema newSchema) {
--- End diff --
Please correct the typo: schemChanged to schemaChanged
---
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 #801: DRILL-5378: Put more information for schema change ...
Posted by gparai <gi...@git.apache.org>.
Github user gparai commented on a diff in the pull request:
https://github.com/apache/drill/pull/801#discussion_r108493691
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/aggregate/HashAggTemplate.java ---
@@ -278,7 +281,7 @@ public void setup(HashAggregate hashAggrConfig, HashTableConfig htConfig, Fragme
}
@Override
- public AggOutcome doWork() {
+ public AggOutcome doWork() throws SchemaChangeException {
--- End diff --
`doWork()` is used elsewhere as well. e.g. StreamingAggregator. Would it make sense to address other `doWork()` s in this PR. Better yet factor out another interface which the HashAggregator/StreamingAggregator implement - this would contain `public AggOutcome doWork() throws SchemaChangeException`
---
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 #801: DRILL-5378: Put more information for schema change excepti...
Posted by jinfengni <gi...@git.apache.org>.
Github user jinfengni commented on the issue:
https://github.com/apache/drill/pull/801
Agreed with @paul-rogers 's comment regarding overuse SchemaChangeException in the code. I actually planned to open a jira, if no one has been opened yet. We currently use SchemaChangeException in many situations, including real schema change or operational error for "something is wrong"
---
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 #801: DRILL-5378: Put more information for schema change ...
Posted by jinfengni <gi...@git.apache.org>.
Github user jinfengni commented on a diff in the pull request:
https://github.com/apache/drill/pull/801#discussion_r108558271
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/exception/SchemaChangeException.java ---
@@ -49,4 +50,16 @@ public SchemaChangeException(String message, Object...objects){
public SchemaChangeException(String message, Throwable cause, Object...objects){
super(String.format(message, objects), cause);
}
+
+ public static SchemaChangeException schemChanged(String message, BatchSchema priorSchema, BatchSchema newSchema) {
--- End diff --
Nice catch. Modified it.
---
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 #801: DRILL-5378: Put more information for schema change ...
Posted by jinfengni <gi...@git.apache.org>.
Github user jinfengni commented on a diff in the pull request:
https://github.com/apache/drill/pull/801#discussion_r108559083
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/aggregate/HashAggTemplate.java ---
@@ -278,7 +281,7 @@ public void setup(HashAggregate hashAggrConfig, HashTableConfig htConfig, Fragme
}
@Override
- public AggOutcome doWork() {
+ public AggOutcome doWork() throws SchemaChangeException {
--- End diff --
You are right that Streaming aggregator can not handle schema change exception currently.
In the current code since Drill's planner put sort operator before streaming aggregator, if the input has schema change, it would hit schema change exception in sort operator first, before come to streaming aggregator.
Anyway, I modified the code in StreamingAggregator, in case Drill planner generate a plan without sort for streaming aggregator.
I also simplified the change, since I realized we do not have to change in AggTemplate; the detailed error message could be produced in AggBatch.
---
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.
---