You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by arina-ielchiieva <gi...@git.apache.org> on 2016/08/07 18:11:21 UTC
[GitHub] drill pull request #560: DRILL-4823: Fix OOM while trying to prune partition...
GitHub user arina-ielchiieva opened a pull request:
https://github.com/apache/drill/pull/560
DRILL-4823: Fix OOM while trying to prune partitions with reasonable \u2026
\u2026data size
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/arina-ielchiieva/drill DRILL-4823
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/drill/pull/560.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 #560
----
commit 778302b957a40ff9e1813da0be5e7f3220376ff8
Author: Arina Ielchiieva <ar...@gmail.com>
Date: 2016-08-03T11:56:43Z
DRILL-4823: Fix OOM while trying to prune partitions with reasonable data size
----
---
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 #560: DRILL-4823: Fix OOM while trying to prune partition...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/drill/pull/560
---
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 #560: DRILL-4823: Fix OOM while trying to prune partitions with ...
Posted by arina-ielchiieva <gi...@git.apache.org>.
Github user arina-ielchiieva commented on the issue:
https://github.com/apache/drill/pull/560
@jinfengni , could you please review?
---
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 #560: DRILL-4823: Fix OOM while trying to prune partitions with ...
Posted by Ben-Zvi <gi...@git.apache.org>.
Github user Ben-Zvi commented on the issue:
https://github.com/apache/drill/pull/560
+1 Could possibly extend the same use of the ConstantValueHolderCache for visitor methods of other types; though this would make the code more cluttered.
---
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 #560: DRILL-4823: Fix OOM while trying to prune partition...
Posted by arina-ielchiieva <gi...@git.apache.org>.
Github user arina-ielchiieva commented on a diff in the pull request:
https://github.com/apache/drill/pull/560#discussion_r86104090
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/interpreter/InterpreterEvaluator.java ---
@@ -378,8 +388,14 @@ public ValueHolder visitDoubleConstant(ValueExpressions.DoubleExpression dExpr,
}
@Override
- public ValueHolder visitQuotedStringConstant(ValueExpressions.QuotedString e, Integer value) throws RuntimeException {
- return ValueHolderHelper.getVarCharHolder(getManagedBufferIfAvailable(), e.value);
+ public ValueHolder visitQuotedStringConstant(final ValueExpressions.QuotedString e, Integer value) throws RuntimeException {
+ return getConstantValueHolder(e.value, new Function<DrillBuf, ValueHolder>() {
--- End diff --
My fault, you are totally right. Now cache stores holders by type.
---
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 #560: DRILL-4823: Fix OOM while trying to prune partition...
Posted by jinfengni <gi...@git.apache.org>.
Github user jinfengni commented on a diff in the pull request:
https://github.com/apache/drill/pull/560#discussion_r85989838
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/interpreter/InterpreterEvaluator.java ---
@@ -378,8 +388,14 @@ public ValueHolder visitDoubleConstant(ValueExpressions.DoubleExpression dExpr,
}
@Override
- public ValueHolder visitQuotedStringConstant(ValueExpressions.QuotedString e, Integer value) throws RuntimeException {
- return ValueHolderHelper.getVarCharHolder(getManagedBufferIfAvailable(), e.value);
+ public ValueHolder visitQuotedStringConstant(final ValueExpressions.QuotedString e, Integer value) throws RuntimeException {
+ return getConstantValueHolder(e.value, new Function<DrillBuf, ValueHolder>() {
--- End diff --
@arina-ielchiieva , I'm not sure if the change makes sense or not.
1. If I have a decimal value, decVal. Let's say decVal.getBigDecimal().toString() return '123'. When I call ValueHolderHelper.getDecimal38Holder(buffer, decExpr.getBigDecimal().toString()), I should return a decimal holder.
2. Now, let's say I have a QuotedString '123', I call ValueHolderHelper.getVarCharHolder(buffer, e.value), since the key e.value is same as '123', and the constant value cache will just return the decimal holder, while I should get a VarcharHolder in stead. Will it cause problem here?
---
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 #560: DRILL-4823: Fix OOM while trying to prune partitions with ...
Posted by amansinha100 <gi...@git.apache.org>.
Github user amansinha100 commented on the issue:
https://github.com/apache/drill/pull/560
@jinfengni could you also take a quick look at this since the code change is in the Interpreter.
---
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 #560: DRILL-4823: Fix OOM while trying to prune partition...
Posted by arina-ielchiieva <gi...@git.apache.org>.
Github user arina-ielchiieva commented on a diff in the pull request:
https://github.com/apache/drill/pull/560#discussion_r85957731
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/interpreter/InterpreterEvaluator.java ---
@@ -378,8 +388,14 @@ public ValueHolder visitDoubleConstant(ValueExpressions.DoubleExpression dExpr,
}
@Override
- public ValueHolder visitQuotedStringConstant(ValueExpressions.QuotedString e, Integer value) throws RuntimeException {
- return ValueHolderHelper.getVarCharHolder(getManagedBufferIfAvailable(), e.value);
+ public ValueHolder visitQuotedStringConstant(final ValueExpressions.QuotedString e, Integer value) throws RuntimeException {
+ return getConstantValueHolder(e.value, new Function<DrillBuf, ValueHolder>() {
--- End diff --
@jinfengni, code does not produce the same keys for decimal and string. We pass different functions for each of them.
Example:
1. return ValueHolderHelper.getDecimal38Holder(buffer, decExpr.getBigDecimal().toString());
2. return ValueHolderHelper.getVarCharHolder(buffer, e.value);
Only buffer is the same for string and decimal with the same value but this is acceptable.
---
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 #560: DRILL-4823: Fix OOM while trying to prune partitions with ...
Posted by jinfengni <gi...@git.apache.org>.
Github user jinfengni commented on the issue:
https://github.com/apache/drill/pull/560
+1
The revised patch looks good to me. @arina-ielchiieva , thanks for your pull request!
---
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 #560: DRILL-4823: Fix OOM while trying to prune partition...
Posted by jinfengni <gi...@git.apache.org>.
Github user jinfengni commented on a diff in the pull request:
https://github.com/apache/drill/pull/560#discussion_r86382118
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java ---
@@ -445,11 +447,16 @@ public PartitionExplorer getPartitionExplorer() {
}
@Override
- public ValueHolder getConstantValueHolder(String value, Function<DrillBuf, ValueHolder> holderInitializer) {
- ValueHolder valueHolder = constantValueHolderCache.get(value);
+ public ValueHolder getConstantValueHolder(String value, MinorType type, Function<DrillBuf, ValueHolder> holderInitializer) {
--- End diff --
it would be nice if we could share the common codes of getConstantValueHolder() in QueryContext/FragmentContext, and wrap them in a helper function. But this is optional.
---
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.
---