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/06/25 02:58:34 UTC
[GitHub] tajo pull request: TAJO-1658: Filter push down to underlying stora...
GitHub user hyunsik opened a pull request:
https://github.com/apache/tajo/pull/613
TAJO-1658: Filter push down to underlying storages
This patch includes TAJO-1659.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/hyunsik/tajo TAJO-1658
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/tajo/pull/613.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 #613
----
commit 90e20d617a9c6af176035f6039d6bbdd8e4f3331
Author: Hyunsik Choi <hy...@apache.org>
Date: 2015-06-24T11:12:37Z
TAJO-1659: Simplify scan iteration in SeqScan.
commit 8d65b2f4a3cb29c5c9c085e6bb7ee48af41a98b4
Author: Hyunsik Choi <hy...@apache.org>
Date: 2015-06-24T11:15:42Z
Add more comments.
commit d6d641606a9ec1f2d497f597aa684c1bb433a809
Author: Hyunsik Choi <hy...@apache.org>
Date: 2015-06-24T21:16:39Z
Moved initialization code.
commit 9e6940356c878b95c2136d46d201db12740747f5
Author: Hyunsik Choi <hy...@apache.org>
Date: 2015-06-24T21:36:43Z
Merge branch 'master' of https://git-wip-us.apache.org/repos/asf/tajo into TAJO-1659
commit 6ff6e05d3423483343cd0e5435da5f65835561e7
Author: Hyunsik Choi <hy...@apache.org>
Date: 2015-06-25T00:41:30Z
TAJO-1658: Filter push down to underlying storages.
commit 67b5654cdf12dde24bb2554fa109f4d1a7c4a957
Author: Hyunsik Choi <hy...@apache.org>
Date: 2015-06-25T00:57:22Z
Merge branch 'master' of https://git-wip-us.apache.org/repos/asf/tajo into TAJO-1658
Conflicts:
tajo-core/src/main/java/org/apache/tajo/engine/planner/physical/SeqScanExec.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.
---
[GitHub] tajo pull request: TAJO-1658: Filter push down to underlying stora...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on the pull request:
https://github.com/apache/tajo/pull/613#issuecomment-116167686
@hyunsik thanks for your patch. It looks good to me.
I left some comments. Please consider them.
---
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-1658: Filter push down to underlying stora...
Posted by hyunsik <gi...@git.apache.org>.
Github user hyunsik commented on a diff in the pull request:
https://github.com/apache/tajo/pull/613#discussion_r33419852
--- Diff: tajo-storage/tajo-storage-common/src/main/java/org/apache/tajo/storage/MergeScanner.java ---
@@ -165,7 +167,7 @@ public boolean isSelectable() {
}
@Override
- public void setSearchCondition(Object expr) {
+ public void setFilter(EvalNode filter) {
--- End diff --
I'll fix it for current implementation. However, we need to know its ambiguity, and the solution should be depend on the final purpose of MergeScanner.
According to its implementation, MergeScanner is chosen if the number of ``fragments`` is more than one.
If MergeScanner is only for intermediate shuffled data, this method should throw an exception. Otherwise, we should implement to push down a filter to its scannner elements.
---
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-1658: Filter push down to underlying stora...
Posted by hyunsik <gi...@git.apache.org>.
Github user hyunsik commented on a diff in the pull request:
https://github.com/apache/tajo/pull/613#discussion_r33420399
--- Diff: tajo-core/src/main/java/org/apache/tajo/engine/planner/physical/SeqScanExec.java ---
@@ -167,23 +162,55 @@ public void init() throws IOException {
projected = outSchema;
}
- initScanner(projected);
- super.init();
-
- if (plan.hasQual()) {
- if (scanner.isProjectable()) {
- qual.bind(context.getEvalContext(), projected);
- } else {
- qual.bind(context.getEvalContext(), inSchema);
- }
+ return projected;
+ }
+ private void initScanIterator() {
+ // We should use FilterScanIterator only if underlying storage does not support filter push down.
--- End diff --
You seem to want to use an unified interface for both index scanner and this work. I'm going to add the storage-related method to determine sub filter expressions which can be push down into a specific storage. Index scanner will be affected by this method in the same manner.
---
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-1658: Filter push down to underlying stora...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on a diff in the pull request:
https://github.com/apache/tajo/pull/613#discussion_r33417550
--- Diff: tajo-core/src/main/java/org/apache/tajo/engine/planner/physical/EmptyScanIterator.java ---
@@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.tajo.engine.planner.physical;
+
+import org.apache.tajo.storage.Tuple;
+
+import java.io.IOException;
+
+/**
+ * Empty Iterator
+ */
+public class EmptyScanIterator implements ScanIterator {
+
+ @Override
+ public boolean hasNext() throws IOException {
+ return false;
+ }
+
+ @Override
+ public Tuple next() throws IOException {
+ throw new IOException(this.getClass().getSimpleName() + "::next() is invoked.");
--- End diff --
I'm wondering that throwing an exception is better than returning a null.
---
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-1658: Filter push down to underlying stora...
Posted by hyunsik <gi...@git.apache.org>.
Github user hyunsik commented on a diff in the pull request:
https://github.com/apache/tajo/pull/613#discussion_r33420036
--- Diff: tajo-storage/tajo-storage-common/src/main/java/org/apache/tajo/storage/AbstractScanner.java ---
@@ -55,7 +56,7 @@ public boolean isSelectable() {
}
@Override
- public void setSearchCondition(Object expr) {
+ public void setFilter(EvalNode filter) {
--- End diff --
Fixed
---
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-1658: Filter push down to underlying stora...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on a diff in the pull request:
https://github.com/apache/tajo/pull/613#discussion_r33417676
--- Diff: tajo-storage/tajo-storage-common/src/main/java/org/apache/tajo/storage/MergeScanner.java ---
@@ -165,7 +167,7 @@ public boolean isSelectable() {
}
@Override
- public void setSearchCondition(Object expr) {
+ public void setFilter(EvalNode filter) {
--- End diff --
It looks that an exception should be thrown.
---
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-1658: Filter push down to underlying stora...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on a diff in the pull request:
https://github.com/apache/tajo/pull/613#discussion_r33417675
--- Diff: tajo-storage/tajo-storage-common/src/main/java/org/apache/tajo/storage/AbstractScanner.java ---
@@ -55,7 +56,7 @@ public boolean isSelectable() {
}
@Override
- public void setSearchCondition(Object expr) {
+ public void setFilter(EvalNode filter) {
--- End diff --
It looks that an exception should be thrown.
---
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-1658: Filter push down to underlying stora...
Posted by hyunsik <gi...@git.apache.org>.
Github user hyunsik commented on the pull request:
https://github.com/apache/tajo/pull/613#issuecomment-116229089
@jihoonson Thank you for your comment. I've updated the 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] tajo pull request: TAJO-1658: Filter push down to underlying stora...
Posted by hyunsik <gi...@git.apache.org>.
Github user hyunsik commented on the pull request:
https://github.com/apache/tajo/pull/613#issuecomment-115058681
This patch also includes some cleanup for table properties.
---
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-1658: Filter push down to underlying stora...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on a diff in the pull request:
https://github.com/apache/tajo/pull/613#discussion_r33417668
--- Diff: tajo-core/src/main/java/org/apache/tajo/engine/planner/physical/SeqScanExec.java ---
@@ -167,23 +162,55 @@ public void init() throws IOException {
projected = outSchema;
}
- initScanner(projected);
- super.init();
-
- if (plan.hasQual()) {
- if (scanner.isProjectable()) {
- qual.bind(context.getEvalContext(), projected);
- } else {
- qual.bind(context.getEvalContext(), inSchema);
- }
+ return projected;
+ }
+ private void initScanIterator() {
+ // We should use FilterScanIterator only if underlying storage does not support filter push down.
--- End diff --
We currently have two kinds of scan operator, sequential scan (```SeqScanExec``` and ```PartitionMergeScanExec```) and index scan (```BSTIndexScanExec```).
In the case of when an index exists for a HDFS table, index scan will do the same operation of ```FullScanIterator```. So, there seems an unified interface for index scan and ```FullScanIterator```.
Maybe, we design a new scanner for HDFS like ```IndexedScanner```. If so, we can also handle index scan in the below code block.
What do you think? Do you have any plans for 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] tajo pull request: TAJO-1658: Filter push down to underlying stora...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/tajo/pull/613
---
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-1658: Filter push down to underlying stora...
Posted by hyunsik <gi...@git.apache.org>.
Github user hyunsik commented on a diff in the pull request:
https://github.com/apache/tajo/pull/613#discussion_r33419572
--- Diff: tajo-core/src/main/java/org/apache/tajo/engine/planner/physical/EmptyScanIterator.java ---
@@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.tajo.engine.planner.physical;
+
+import org.apache.tajo.storage.Tuple;
+
+import java.io.IOException;
+
+/**
+ * Empty Iterator
+ */
+public class EmptyScanIterator implements ScanIterator {
+
+ @Override
+ public boolean hasNext() throws IOException {
+ return false;
+ }
+
+ @Override
+ public Tuple next() throws IOException {
+ throw new IOException(this.getClass().getSimpleName() + "::next() is invoked.");
--- End diff --
I think that invoking next() is illegal state because it means that checking hasNext() is omitted.
---
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-1658: Filter push down to underlying stora...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on the pull request:
https://github.com/apache/tajo/pull/613#issuecomment-116231508
Thanks for rapid reply.
+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.
---