You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@tajo.apache.org by navis <gi...@git.apache.org> on 2015/03/23 14:43:46 UTC
[GitHub] tajo pull request: TAJO-1436 Add Bind method to EvalNode
GitHub user navis opened a pull request:
https://github.com/apache/tajo/pull/456
TAJO-1436 Add Bind method to EvalNode
eval(Schema schema, Tuple tuple) --> bind(Schema schema) + eval(Tuple tuple)
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/navis/tajo TAJO-1436
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/tajo/pull/456.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 #456
----
commit 65d68fd466e208f0eaaff973f862ae61dce873ee
Author: navis.ryu <na...@apache.org>
Date: 2015-03-23T13:42:11Z
TAJO-1436 Add Bind method to EvalNode
----
---
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] tajo pull request: TAJO-1436 Add Bind method to EvalNode
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on the pull request:
https://github.com/apache/tajo/pull/456#issuecomment-88814411
Oh, you have already rebased. Sorry. I'll review ASAP.
---
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] tajo pull request: TAJO-1436 Add Bind method to EvalNode
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on the pull request:
https://github.com/apache/tajo/pull/456#issuecomment-88851633
+1
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] tajo pull request: TAJO-1436 Add Bind method to EvalNode
Posted by navis <gi...@git.apache.org>.
Github user navis commented on a diff in the pull request:
https://github.com/apache/tajo/pull/456#discussion_r27372699
--- Diff: tajo-plan/src/main/java/org/apache/tajo/plan/expr/FunctionEval.java ---
@@ -40,13 +40,27 @@
@Expose protected FunctionDesc funcDesc;
@Expose protected EvalNode [] argEvals;
+ private transient Tuple params;
--- End diff --
It's transient field as it declared, which should not be included in hash or equals calculation.
---
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] tajo pull request: TAJO-1436 Add Bind method to EvalNode
Posted by navis <gi...@git.apache.org>.
Github user navis commented on a diff in the pull request:
https://github.com/apache/tajo/pull/456#discussion_r27371459
--- Diff: tajo-core/src/main/java/org/apache/tajo/engine/planner/physical/HashLeftOuterJoinExec.java ---
@@ -120,11 +120,11 @@ public HashLeftOuterJoinExec(TaskAttemptContext context, JoinNode plan, Physical
leftKeyTuple = new VTuple(leftKeyList.length);
rightNumCols = rightChild.getSchema().size();
- }
- @Override
- protected void compile() {
- joinQual = context.getPrecompiledEval(inSchema, joinQual);
--- End diff --
Yes, mistake. Nice catch.
---
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] tajo pull request: TAJO-1436 Add Bind method to EvalNode
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on a diff in the pull request:
https://github.com/apache/tajo/pull/456#discussion_r27371248
--- Diff: tajo-core/src/main/java/org/apache/tajo/engine/planner/physical/SeqScanExec.java ---
@@ -69,9 +70,8 @@ public SeqScanExec(TaskAttemptContext context, ScanNode plan,
this.qual = plan.getQual();
this.fragments = fragments;
- if (fragments != null
- && plan.getTableDesc().hasPartition()
--- End diff --
The variable ```fragments``` can be null, and ```rewriteColumnPartitionedTableSchema()``` should not be called in that 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] tajo pull request: TAJO-1436 Add Bind method to EvalNode
Posted by navis <gi...@git.apache.org>.
Github user navis commented on a diff in the pull request:
https://github.com/apache/tajo/pull/456#discussion_r27372731
--- Diff: tajo-core/src/main/java/org/apache/tajo/engine/planner/physical/SeqScanExec.java ---
@@ -197,6 +206,9 @@ public Tuple next() throws IOException {
if (fragments == null) {
return null;
}
+ if (scanner == null) {
+ System.out.println("[SeqScanExec/next] ");
--- End diff --
done
---
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] tajo pull request: TAJO-1436 Add Bind method to EvalNode
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/tajo/pull/456
---
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] tajo pull request: TAJO-1436 Add Bind method to EvalNode
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on the pull request:
https://github.com/apache/tajo/pull/456#issuecomment-88751004
Would you please rebase?
---
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] tajo pull request: TAJO-1436 Add Bind method to EvalNode
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on a diff in the pull request:
https://github.com/apache/tajo/pull/456#discussion_r27372114
--- Diff: tajo-plan/src/main/java/org/apache/tajo/plan/expr/FunctionEval.java ---
@@ -40,13 +40,27 @@
@Expose protected FunctionDesc funcDesc;
@Expose protected EvalNode [] argEvals;
+ private transient Tuple params;
--- End diff --
This variable is missed in ```hashCode()```, ```clone()```, and ```equals()``` functions.
---
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] tajo pull request: TAJO-1436 Add Bind method to EvalNode
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on a diff in the pull request:
https://github.com/apache/tajo/pull/456#discussion_r27371479
--- Diff: tajo-core/src/main/java/org/apache/tajo/engine/planner/physical/SeqScanExec.java ---
@@ -197,6 +206,9 @@ public Tuple next() throws IOException {
if (fragments == null) {
return null;
}
+ if (scanner == null) {
+ System.out.println("[SeqScanExec/next] ");
--- End diff --
Please remove debug codes.
---
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] tajo pull request: TAJO-1436 Add Bind method to EvalNode
Posted by navis <gi...@git.apache.org>.
Github user navis commented on a diff in the pull request:
https://github.com/apache/tajo/pull/456#discussion_r27371654
--- Diff: tajo-core/src/main/java/org/apache/tajo/engine/planner/physical/SeqScanExec.java ---
@@ -69,9 +70,8 @@ public SeqScanExec(TaskAttemptContext context, ScanNode plan,
this.qual = plan.getQual();
this.fragments = fragments;
- if (fragments != null
- && plan.getTableDesc().hasPartition()
--- End diff --
Even fragments is null, partition columns in expr should be replaced with null. Without this, some tests fails with exception("No Such Column Reference").
---
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] tajo pull request: TAJO-1436 Add Bind method to EvalNode
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on the pull request:
https://github.com/apache/tajo/pull/456#issuecomment-87618800
Thanks @navis. The patch looks good.
I have some additional comments as follows.
* Changes on NettyClientBase look for https://issues.apache.org/jira/browse/TAJO-1424. If so, it would be better to resolve in that issue. Please remove them.
* The bind() function must be called before eval(). So, it would be much great if you add a routine to check the binding state.
---
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] tajo pull request: TAJO-1436 Add Bind method to EvalNode
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on a diff in the pull request:
https://github.com/apache/tajo/pull/456#discussion_r27373655
--- Diff: tajo-plan/src/main/java/org/apache/tajo/plan/expr/FunctionEval.java ---
@@ -40,13 +40,27 @@
@Expose protected FunctionDesc funcDesc;
@Expose protected EvalNode [] argEvals;
+ private transient Tuple params;
--- End diff --
Oops. Sorry.
---
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] tajo pull request: TAJO-1436 Add Bind method to EvalNode
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on a diff in the pull request:
https://github.com/apache/tajo/pull/456#discussion_r27370991
--- Diff: tajo-core/src/main/java/org/apache/tajo/engine/planner/physical/HashLeftOuterJoinExec.java ---
@@ -120,11 +120,11 @@ public HashLeftOuterJoinExec(TaskAttemptContext context, JoinNode plan, Physical
leftKeyTuple = new VTuple(leftKeyList.length);
rightNumCols = rightChild.getSchema().size();
- }
- @Override
- protected void compile() {
- joinQual = context.getPrecompiledEval(inSchema, joinQual);
--- End diff --
The compile() function should not be removed.
---
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] tajo pull request: TAJO-1436 Add Bind method to EvalNode
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on the pull request:
https://github.com/apache/tajo/pull/456#issuecomment-88416293
Ok. I created another issue for bind check. https://issues.apache.org/jira/browse/TAJO-1499
---
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] tajo pull request: TAJO-1436 Add Bind method to EvalNode
Posted by navis <gi...@git.apache.org>.
Github user navis commented on the pull request:
https://github.com/apache/tajo/pull/456#issuecomment-88303781
I've also thought about that (bind check), but thinking that the intention of this issue was for removing any non-working code from eval, I decided to remove it. Opinion?
I'll fix TAJO-1424, which seemed abondoned for a long 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.
---