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 2016/02/17 16:20:16 UTC
[GitHub] drill pull request: DRILL-4387: GroupScan or ScanBatchCreator shou...
GitHub user jinfengni opened a pull request:
https://github.com/apache/drill/pull/379
DRILL-4387: GroupScan or ScanBatchCreator should not use star column …
…in case of skipAll query.
The skipAll query should be handled in RecordReader.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/jinfengni/incubator-drill DRILL-4387
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/drill/pull/379.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 #379
----
commit 5c1edc42dcad6c3b5943424b9a8373cf6ff51753
Author: Jinfeng Ni <jn...@apache.org>
Date: 2016-02-12T22:18:59Z
DRILL-4387: GroupScan or ScanBatchCreator should not use star column in case of skipAll query.
The skipAll query should be handled in RecordReader.
----
---
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: DRILL-4387: GroupScan or ScanBatchCreator shou...
Posted by jaltekruse <gi...@git.apache.org>.
Github user jaltekruse commented on the pull request:
https://github.com/apache/drill/pull/379#issuecomment-212534349
@jinfengni looks like this was merged, can you close the PR?
---
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: DRILL-4387: GroupScan or ScanBatchCreator shou...
Posted by amansinha100 <gi...@git.apache.org>.
Github user amansinha100 commented on the pull request:
https://github.com/apache/drill/pull/379#issuecomment-185934019
LGTM +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 pull request: DRILL-4387: GroupScan or ScanBatchCreator shou...
Posted by amansinha100 <gi...@git.apache.org>.
Github user amansinha100 commented on a diff in the pull request:
https://github.com/apache/drill/pull/379#discussion_r53340898
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/ParquetScanBatchCreator.java ---
@@ -87,9 +87,6 @@ public ScanBatch getBatch(FragmentContext context, ParquetRowGroupScan rowGroupS
newColumns.add(column);
}
}
- if (newColumns.isEmpty()) {
--- End diff --
So, to clarify, the reason you removed the check for newColumns.isEmpty() is that if the column list is empty, the underlying ParquetRecordReader will handle it correctly by produce 1 default column (probably a NullableInt column) ?
Was this check for isEmpty() only present in the Parquet scan ? or should other readers need modification too ?
I think it would be good to add comments about how the NULL and empty column list are being handled by each data source.
---
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: DRILL-4387: GroupScan or ScanBatchCreator shou...
Posted by amansinha100 <gi...@git.apache.org>.
Github user amansinha100 commented on a diff in the pull request:
https://github.com/apache/drill/pull/379#discussion_r53386823
--- Diff: contrib/storage-hbase/src/main/java/org/apache/drill/exec/store/hbase/HBaseGroupScan.java ---
@@ -34,6 +34,7 @@
import java.util.concurrent.TimeUnit;
import com.fasterxml.jackson.annotation.JsonCreator;
+import com.google.common.base.Objects;
--- End diff --
unnecessary import ?
---
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: DRILL-4387: GroupScan or ScanBatchCreator shou...
Posted by jinfengni <gi...@git.apache.org>.
Github user jinfengni commented on a diff in the pull request:
https://github.com/apache/drill/pull/379#discussion_r53366236
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/ParquetScanBatchCreator.java ---
@@ -87,9 +87,6 @@ public ScanBatch getBatch(FragmentContext context, ParquetRowGroupScan rowGroupS
newColumns.add(column);
}
}
- if (newColumns.isEmpty()) {
--- End diff --
@amansinha100 , I made slightly change to the patch to address the comments. Could you please take another look?
Thanks!
---
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: DRILL-4387: GroupScan or ScanBatchCreator shou...
Posted by amansinha100 <gi...@git.apache.org>.
Github user amansinha100 commented on a diff in the pull request:
https://github.com/apache/drill/pull/379#discussion_r53335185
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/physical/base/GroupScan.java ---
@@ -35,6 +35,8 @@
public interface GroupScan extends Scan, HasAffinity{
public static final List<SchemaPath> ALL_COLUMNS = ImmutableList.of(SchemaPath.getSimplePath("*"));
+ public static final List<SchemaPath> EMPTY_COLUMNS = ImmutableList.of();
--- End diff --
This static constant does not seem to be referenced anywhere ?
---
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: DRILL-4387: GroupScan or ScanBatchCreator shou...
Posted by jinfengni <gi...@git.apache.org>.
Github user jinfengni commented on a diff in the pull request:
https://github.com/apache/drill/pull/379#discussion_r53361870
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/ParquetScanBatchCreator.java ---
@@ -87,9 +87,6 @@ public ScanBatch getBatch(FragmentContext context, ParquetRowGroupScan rowGroupS
newColumns.add(column);
}
}
- if (newColumns.isEmpty()) {
--- End diff --
I went through all the ScanBatchCreator in Drill's code base. Seems ParquetScanBatchCreator is the only one that is converting an empty column list to ALL_COLUMNS. Looking at the history, seems DRILL-1845 added the code, probably just to make it work in parquet for skipAll query.
With the patch of DRILL-4279, parquet record reader would be able to handle empty column list.
Besides ParquetScanBatchCreator, this patch also modifies HBaseGroupScan, EasyGroupScan where it originally interprets empty column lists into ALL_COLUMNS.
I'll add some comment to the code to clarify the different meaning of NULL and empty column list.
---
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: DRILL-4387: GroupScan or ScanBatchCreator shou...
Posted by jinfengni <gi...@git.apache.org>.
Github user jinfengni commented on a diff in the pull request:
https://github.com/apache/drill/pull/379#discussion_r53360482
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/physical/base/GroupScan.java ---
@@ -35,6 +35,8 @@
public interface GroupScan extends Scan, HasAffinity{
public static final List<SchemaPath> ALL_COLUMNS = ImmutableList.of(SchemaPath.getSimplePath("*"));
+ public static final List<SchemaPath> EMPTY_COLUMNS = ImmutableList.of();
--- End diff --
Nice catch. It's no longer needed. (Originally, I intended to convert NULL to empty_columns. But not it's not necessary). I'll remove that. 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 pull request: DRILL-4387: GroupScan or ScanBatchCreator shou...
Posted by jinfengni <gi...@git.apache.org>.
Github user jinfengni closed the pull request at:
https://github.com/apache/drill/pull/379
---
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: DRILL-4387: GroupScan or ScanBatchCreator shou...
Posted by jinfengni <gi...@git.apache.org>.
Github user jinfengni commented on a diff in the pull request:
https://github.com/apache/drill/pull/379#discussion_r53387614
--- Diff: contrib/storage-hbase/src/main/java/org/apache/drill/exec/store/hbase/HBaseGroupScan.java ---
@@ -34,6 +34,7 @@
import java.util.concurrent.TimeUnit;
import com.fasterxml.jackson.annotation.JsonCreator;
+import com.google.common.base.Objects;
--- End diff --
right. I'll remove these unused imports. Thanks.
---
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.
---