You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@tajo.apache.org by hyunsik <gi...@git.apache.org> on 2015/11/24 10:29:12 UTC
[GitHub] tajo pull request: TAJO-1987: Add stream API to Projectable and re...
GitHub user hyunsik opened a pull request:
https://github.com/apache/tajo/pull/874
TAJO-1987: Add stream API to Projectable and refactor its usage codes.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/hyunsik/tajo TAJO-1987
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/tajo/pull/874.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 #874
----
commit 852633bd7f3f43204afc4c850f0d06e41fcb5d58
Author: Hyunsik Choi <hy...@apache.org>
Date: 2015-11-24T09:28:47Z
TAJO-1987: Add stream API to Projectable and refactor its usage 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-1987: Add stream API to Projectable and re...
Posted by blrunner <gi...@git.apache.org>.
Github user blrunner commented on the pull request:
https://github.com/apache/tajo/pull/874#issuecomment-159787446
The patch looks good to me. I leaved a few trivial comments.
---
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 #874: TAJO-1987: Add stream API to Projectable and refacto...
Posted by hyunsik <gi...@git.apache.org>.
Github user hyunsik closed the pull request at:
https://github.com/apache/tajo/pull/874
---
[GitHub] tajo pull request: TAJO-1987: Add stream API to Projectable and re...
Posted by blrunner <gi...@git.apache.org>.
Github user blrunner commented on a diff in the pull request:
https://github.com/apache/tajo/pull/874#discussion_r45939937
--- Diff: tajo-plan/src/main/java/org/apache/tajo/plan/verifier/LogicalPlanVerifier.java ---
@@ -90,9 +90,7 @@ public LogicalNode visitProjection(Context state, LogicalPlan plan, LogicalPlan.
ProjectionNode node, Stack<LogicalNode> stack) throws TajoException {
super.visitProjection(state, plan, block, node, stack);
- for (Target target : node.getTargets()) {
--- End diff --
Could you remove ```Target``` in package import 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 issue #874: TAJO-1987: Add stream API to Projectable and refactor its u...
Posted by hyunsik <gi...@git.apache.org>.
Github user hyunsik commented on the issue:
https://github.com/apache/tajo/pull/874
Close this issue because most of improvements are already included in TAJO-2108.
---
[GitHub] tajo pull request: TAJO-1987: Add stream API to Projectable and re...
Posted by hyunsik <gi...@git.apache.org>.
Github user hyunsik commented on the pull request:
https://github.com/apache/tajo/pull/874#issuecomment-161533876
I reflected the comments and rebased.
---
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-1987: Add stream API to Projectable and re...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on the pull request:
https://github.com/apache/tajo/pull/874#issuecomment-164320517
This patch looks good to me. Would you rebase against master?
---
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-1987: Add stream API to Projectable and re...
Posted by hyunsik <gi...@git.apache.org>.
Github user hyunsik commented on a diff in the pull request:
https://github.com/apache/tajo/pull/874#discussion_r46516484
--- Diff: tajo-plan/src/main/java/org/apache/tajo/plan/logical/ProjectionNode.java ---
@@ -64,8 +65,13 @@ public void setTargets(List<Target> targets) {
public List<Target> getTargets() {
return this.targets;
}
-
- public void setChild(LogicalNode subNode) {
+
+ @Override
+ public Stream<Target> targets() {
--- End diff --
This is because we still need List allowing access elements by index in some 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-1987: Add stream API to Projectable and re...
Posted by blrunner <gi...@git.apache.org>.
Github user blrunner commented on a diff in the pull request:
https://github.com/apache/tajo/pull/874#discussion_r45939647
--- Diff: tajo-plan/src/main/java/org/apache/tajo/plan/logical/ScanNode.java ---
@@ -32,7 +32,10 @@
import org.apache.tajo.util.TUtil;
import java.util.ArrayList;
+import java.util.Collections;
import java.util.List;
+import java.util.stream.Stream;
+import java.util.stream.StreamSupport;
--- End diff --
It seems to be an unused package.
---
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-1987: Add stream API to Projectable and re...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on a diff in the pull request:
https://github.com/apache/tajo/pull/874#discussion_r46014921
--- Diff: tajo-plan/src/main/java/org/apache/tajo/plan/logical/ProjectionNode.java ---
@@ -64,8 +65,13 @@ public void setTargets(List<Target> targets) {
public List<Target> getTargets() {
return this.targets;
}
-
- public void setChild(LogicalNode subNode) {
+
+ @Override
+ public Stream<Target> targets() {
--- End diff --
Would you share why you keep the old method ```getTargets()```? For the compatibility with 1.7?
---
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-1987: Add stream API to Projectable and re...
Posted by blrunner <gi...@git.apache.org>.
Github user blrunner commented on the pull request:
https://github.com/apache/tajo/pull/874#issuecomment-159782283
I'm reviewing 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.
---