You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by ppadma <gi...@git.apache.org> on 2016/09/27 00:48:40 UTC
[GitHub] drill pull request #597: DRILL-4905: Push down the LIMIT to the parquet read...
GitHub user ppadma opened a pull request:
https://github.com/apache/drill/pull/597
DRILL-4905: Push down the LIMIT to the parquet reader scan.
For limit N query, where N is less than current default record batchSize (256K for all fixedlength, 32K otherwise), we still end up reading all 256K/32K rows from disk if rowGroup has that many rows. This causes performance degradation especially when there are large number of columns.
This fix tries to address this problem by changing the record batchSize parquet record reader uses so we don't read more than what is needed.
Also, added a sys option (store.parquet.record_batch_size) to be able to set record batch size.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ppadma/drill DRILL-4905
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/drill/pull/597.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 #597
----
commit cd665ebdba11f8685ba446f5ec535c81ddd6edc7
Author: Padma Penumarthy <pp...@ppenumarthy-e653-mpr13.local>
Date: 2016-09-26T17:51:07Z
DRILL-4905: Push down the LIMIT to the parquet reader scan to limit the numbers of records read
----
---
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 #597: DRILL-4905: Push down the LIMIT to the parquet read...
Posted by ppadma <gi...@git.apache.org>.
Github user ppadma commented on a diff in the pull request:
https://github.com/apache/drill/pull/597#discussion_r80788614
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/ParquetGroupScan.java ---
@@ -899,6 +907,16 @@ public FileGroupScan clone(FileSelection selection) throws IOException {
return newScan;
}
+ // clone to create new groupscan with new file selection and batchSize.
+ public ParquetGroupScan clone(FileSelection selection, int batchSize) throws IOException {
+ ParquetGroupScan newScan = new ParquetGroupScan(this);
+ newScan.modifyFileSelection(selection);
+ newScan.cacheFileRoot = selection.cacheFileRoot;
--- 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] drill pull request #597: DRILL-4905: Push down the LIMIT to the parquet read...
Posted by jinfengni <gi...@git.apache.org>.
Github user jinfengni commented on a diff in the pull request:
https://github.com/apache/drill/pull/597#discussion_r80968605
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/ExecConstants.java ---
@@ -125,13 +125,14 @@
String PARQUET_WRITER_ENABLE_DICTIONARY_ENCODING = "store.parquet.enable_dictionary_encoding";
OptionValidator PARQUET_WRITER_ENABLE_DICTIONARY_ENCODING_VALIDATOR = new BooleanValidator(
PARQUET_WRITER_ENABLE_DICTIONARY_ENCODING, false);
-
String PARQUET_VECTOR_FILL_THRESHOLD = "store.parquet.vector_fill_threshold";
OptionValidator PARQUET_VECTOR_FILL_THRESHOLD_VALIDATOR = new PositiveLongValidator(PARQUET_VECTOR_FILL_THRESHOLD, 99l, 85l);
String PARQUET_VECTOR_FILL_CHECK_THRESHOLD = "store.parquet.vector_fill_check_threshold";
OptionValidator PARQUET_VECTOR_FILL_CHECK_THRESHOLD_VALIDATOR = new PositiveLongValidator(PARQUET_VECTOR_FILL_CHECK_THRESHOLD, 100l, 10l);
String PARQUET_NEW_RECORD_READER = "store.parquet.use_new_reader";
OptionValidator PARQUET_RECORD_READER_IMPLEMENTATION_VALIDATOR = new BooleanValidator(PARQUET_NEW_RECORD_READER, false);
+ String PARQUET_RECORD_BATCH_SIZE = "store.parquet.record_batch_size";
+ OptionValidator PARQUET_RECORD_BATCH_SIZE_VALIDATOR = new LongValidator(PARQUET_RECORD_BATCH_SIZE, 256*1024);
--- End diff --
If you want to specify the maximum of allowed batch_size, you probably have to use PositiveLongValidator, which will allow u to specify max.
---
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 #597: DRILL-4905: Push down the LIMIT to the parquet read...
Posted by jinfengni <gi...@git.apache.org>.
Github user jinfengni commented on a diff in the pull request:
https://github.com/apache/drill/pull/597#discussion_r83334160
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/columnreaders/ParquetRecordReader.java ---
@@ -115,17 +118,30 @@
public ParquetRecordReader(FragmentContext fragmentContext,
String path,
int rowGroupIndex,
+ long numRecordsToRead,
FileSystem fs,
CodecFactory codecFactory,
ParquetMetadata footer,
List<SchemaPath> columns) throws ExecutionSetupException {
- this(fragmentContext, DEFAULT_BATCH_LENGTH_IN_BITS, path, rowGroupIndex, fs, codecFactory, footer,
- columns);
+ this(fragmentContext, DEFAULT_BATCH_LENGTH_IN_BITS, numRecordsToRead,
+ path, rowGroupIndex, fs, codecFactory, footer, columns);
+ }
+
+ public ParquetRecordReader(FragmentContext fragmentContext,
+ String path,
+ int rowGroupIndex,
+ FileSystem fs,
+ CodecFactory codecFactory,
+ ParquetMetadata footer,
+ List<SchemaPath> columns) throws ExecutionSetupException {
+ this(fragmentContext, DEFAULT_BATCH_LENGTH_IN_BITS, DEFAULT_RECORDS_TO_READ_NOT_SPECIFIED,
--- End diff --
I think we can get rid of DEFAULT_RECORDS_TO_READ_NOT_SPECIFIED. If numRecordsToRead is not specified, just use rowCount in the row Group.
---
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 #597: DRILL-4905: Push down the LIMIT to the parquet read...
Posted by gparai <gi...@git.apache.org>.
Github user gparai commented on a diff in the pull request:
https://github.com/apache/drill/pull/597#discussion_r80612914
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/ParquetGroupScan.java ---
@@ -899,6 +907,16 @@ public FileGroupScan clone(FileSelection selection) throws IOException {
return newScan;
}
+ // clone to create new groupscan with new file selection and batchSize.
+ public ParquetGroupScan clone(FileSelection selection, int batchSize) throws IOException {
+ ParquetGroupScan newScan = new ParquetGroupScan(this);
+ newScan.modifyFileSelection(selection);
+ newScan.cacheFileRoot = selection.cacheFileRoot;
--- End diff --
Should we have a setter method 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] drill pull request #597: DRILL-4905: Push down the LIMIT to the parquet read...
Posted by ppadma <gi...@git.apache.org>.
Github user ppadma commented on a diff in the pull request:
https://github.com/apache/drill/pull/597#discussion_r80788617
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/ParquetGroupScan.java ---
@@ -899,6 +907,16 @@ public FileGroupScan clone(FileSelection selection) throws IOException {
return newScan;
}
+ // clone to create new groupscan with new file selection and batchSize.
+ public ParquetGroupScan clone(FileSelection selection, int batchSize) throws IOException {
+ ParquetGroupScan newScan = new ParquetGroupScan(this);
+ newScan.modifyFileSelection(selection);
+ newScan.cacheFileRoot = selection.cacheFileRoot;
+ newScan.init(selection.getMetaContext());
+ newScan.batchSize = batchSize;
--- 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] drill pull request #597: DRILL-4905: Push down the LIMIT to the parquet read...
Posted by jinfengni <gi...@git.apache.org>.
Github user jinfengni commented on a diff in the pull request:
https://github.com/apache/drill/pull/597#discussion_r80796011
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/ParquetGroupScan.java ---
@@ -115,6 +115,8 @@
private List<RowGroupInfo> rowGroupInfos;
private Metadata.ParquetTableMetadataBase parquetTableMetadata = null;
private String cacheFileRoot = null;
+ private int batchSize;
+ private static final int DEFAULT_BATCH_LENGTH = 256 * 1024;
--- End diff --
Are you referring to code here:
{code}
// Pick the minimum of recordsPerBatch calculated above, batchSize we got from rowGroupScan (based on limit)
// and user configured batchSize value.
recordsPerBatch = (int) Math.min(Math.min(recordsPerBatch, batchSize),
fragmentContext.getOptions().getOption(ExecConstants.PARQUET_RECORD_BATCH_SIZE).num_val.intValue());
{code}
If I understand correctly, batchSize in ParquetRecordReader comes from ParquetRowGroupScan, which comes from ParquetGroupScan, which is set to DEFAULT_BATCH_LENGTH. If I have a RG with 512K rows, and I set "store.parquet.record_batch_size" to be 512K, will your code honor this 512 batch size, or will it use DEFAULT_BATCH_LENGTH since it's smallest?
Also, if "store.parquet.record_batch_size" is set to be different from DEFAULT_BATCH_LENGTH, why would we still use DEFAULT_BATCH_LENGTH in ParquetGroupScan / ParquetRowGroupScan? People might be confused if they look at the serialized physical plan, which shows "batchSize = DEFAULT_BATCH_LENGTH.
---
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 #597: DRILL-4905: Push down the LIMIT to the parquet read...
Posted by gparai <gi...@git.apache.org>.
Github user gparai commented on a diff in the pull request:
https://github.com/apache/drill/pull/597#discussion_r80612967
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/ParquetGroupScan.java ---
@@ -899,6 +907,16 @@ public FileGroupScan clone(FileSelection selection) throws IOException {
return newScan;
}
+ // clone to create new groupscan with new file selection and batchSize.
+ public ParquetGroupScan clone(FileSelection selection, int batchSize) throws IOException {
+ ParquetGroupScan newScan = new ParquetGroupScan(this);
+ newScan.modifyFileSelection(selection);
+ newScan.cacheFileRoot = selection.cacheFileRoot;
+ newScan.init(selection.getMetaContext());
+ newScan.batchSize = batchSize;
--- End diff --
Should we have a setter method for this? Or builder pattern?
---
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 #597: DRILL-4905: Push down the LIMIT to the parquet read...
Posted by jinfengni <gi...@git.apache.org>.
Github user jinfengni commented on a diff in the pull request:
https://github.com/apache/drill/pull/597#discussion_r81194437
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/ParquetGroupScan.java ---
@@ -926,16 +944,22 @@ public GroupScan applyLimit(long maxRecords) {
fileNames.add(rowGroupInfo.getPath());
}
- if (fileNames.size() == fileSet.size() ) {
+ // If there is no change in fileSet and maxRecords is >= batchSize, no need to create new groupScan.
+ if (fileNames.size() == fileSet.size() && (maxRecords >= batchSize) ) {
// There is no reduction of rowGroups. Return the original groupScan.
logger.debug("applyLimit() does not apply!");
return null;
}
+ // If limit maxRecords is less than batchSize, update batchSize to the limit size.
+ if (maxRecords < batchSize) {
+ batchSize = (int) maxRecords;
--- End diff --
Maybe I miss something here. But seems to me as long as you declare "long batchSize" in ParquetGroupScan/RowGroupScan (All the code seems to be new code), you do not have to have this cast.
---
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 #597: DRILL-4905: Push down the LIMIT to the parquet read...
Posted by jinfengni <gi...@git.apache.org>.
Github user jinfengni commented on a diff in the pull request:
https://github.com/apache/drill/pull/597#discussion_r81196419
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/ParquetGroupScan.java ---
@@ -894,11 +910,21 @@ public GroupScan clone(List<SchemaPath> columns) {
public FileGroupScan clone(FileSelection selection) throws IOException {
ParquetGroupScan newScan = new ParquetGroupScan(this);
newScan.modifyFileSelection(selection);
- newScan.cacheFileRoot = selection.cacheFileRoot;
+ newScan.setCacheFileRoot(selection.cacheFileRoot);
newScan.init(selection.getMetaContext());
return newScan;
}
+ // clone to create new groupscan with new file selection and batchSize.
+ public ParquetGroupScan clone(FileSelection selection, int batchSize) throws IOException {
+ ParquetGroupScan newScan = new ParquetGroupScan(this);
--- End diff --
Is it possible to share the code with the method in line 910 - 914?
---
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 #597: DRILL-4905: Push down the LIMIT to the parquet read...
Posted by gparai <gi...@git.apache.org>.
Github user gparai commented on a diff in the pull request:
https://github.com/apache/drill/pull/597#discussion_r80613372
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/ParquetGroupScan.java ---
@@ -926,16 +944,22 @@ public GroupScan applyLimit(long maxRecords) {
fileNames.add(rowGroupInfo.getPath());
}
- if (fileNames.size() == fileSet.size() ) {
+ // If there is no change in fileSet and maxRecords is >= batchSize, no need to create new groupScan.
+ if (fileNames.size() == fileSet.size() && (maxRecords >= batchSize) ) {
// There is no reduction of rowGroups. Return the original groupScan.
logger.debug("applyLimit() does not apply!");
return null;
}
+ // If limit maxRecords is less than batchSize, update batchSize to the limit size.
+ if (maxRecords < batchSize) {
+ batchSize = (int) maxRecords;
--- End diff --
Can we avoid the cast?
---
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 #597: DRILL-4905: Push down the LIMIT to the parquet read...
Posted by ppadma <gi...@git.apache.org>.
Github user ppadma commented on a diff in the pull request:
https://github.com/apache/drill/pull/597#discussion_r80990803
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/ParquetGroupScan.java ---
@@ -115,6 +115,8 @@
private List<RowGroupInfo> rowGroupInfos;
private Metadata.ParquetTableMetadataBase parquetTableMetadata = null;
private String cacheFileRoot = null;
+ private int batchSize;
+ private static final int DEFAULT_BATCH_LENGTH = 256 * 1024;
--- End diff --
Done. Please review new diffs
---
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 #597: DRILL-4905: Push down the LIMIT to the parquet reader scan...
Posted by ppadma <gi...@git.apache.org>.
Github user ppadma commented on the issue:
https://github.com/apache/drill/pull/597
updated with new diffs. 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 pull request #597: DRILL-4905: Push down the LIMIT to the parquet read...
Posted by ppadma <gi...@git.apache.org>.
Github user ppadma commented on a diff in the pull request:
https://github.com/apache/drill/pull/597#discussion_r80756694
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/ParquetGroupScan.java ---
@@ -926,16 +944,22 @@ public GroupScan applyLimit(long maxRecords) {
fileNames.add(rowGroupInfo.getPath());
}
- if (fileNames.size() == fileSet.size() ) {
+ // If there is no change in fileSet and maxRecords is >= batchSize, no need to create new groupScan.
+ if (fileNames.size() == fileSet.size() && (maxRecords >= batchSize) ) {
// There is no reduction of rowGroups. Return the original groupScan.
logger.debug("applyLimit() does not apply!");
return null;
}
+ // If limit maxRecords is less than batchSize, update batchSize to the limit size.
+ if (maxRecords < batchSize) {
+ batchSize = (int) maxRecords;
--- End diff --
I tried to avoid this casting. To fix this, have to change code in bunch of places. I was trying to keep code changes minimal so no new regressions are introduced.
---
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 #597: DRILL-4905: Push down the LIMIT to the parquet read...
Posted by ppadma <gi...@git.apache.org>.
Github user ppadma commented on a diff in the pull request:
https://github.com/apache/drill/pull/597#discussion_r81244611
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/ParquetGroupScan.java ---
@@ -864,6 +872,14 @@ public String getDigest() {
return toString();
}
+ public void setCacheFileRoot(String cacheFileRoot) {
+ this.cacheFileRoot = cacheFileRoot;
+ }
+
+ public void setBatchSize(long batchSize) {
+ this.recommendedBatchSize = batchSize;
--- End diff --
checking against current value is fine for now. Adding new methods will be a bit outside the scope of this JIRA.
---
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 #597: DRILL-4905: Push down the LIMIT to the parquet read...
Posted by ppadma <gi...@git.apache.org>.
Github user ppadma commented on a diff in the pull request:
https://github.com/apache/drill/pull/597#discussion_r80788478
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/ParquetScanBatchCreator.java ---
@@ -107,7 +107,7 @@ public ScanBatch getBatch(FragmentContext context, ParquetRowGroupScan rowGroupS
if (!context.getOptions().getOption(ExecConstants.PARQUET_NEW_RECORD_READER).bool_val && !isComplex(footers.get(e.getPath()))) {
readers.add(
new ParquetRecordReader(
- context, e.getPath(), e.getRowGroupIndex(), fs,
+ context, rowGroupScan.getBatchSize(), e.getPath(), e.getRowGroupIndex(), fs,
--- 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] drill pull request #597: DRILL-4905: Push down the LIMIT to the parquet read...
Posted by gparai <gi...@git.apache.org>.
Github user gparai commented on a diff in the pull request:
https://github.com/apache/drill/pull/597#discussion_r81232963
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/ParquetGroupScan.java ---
@@ -864,6 +872,14 @@ public String getDigest() {
return toString();
}
+ public void setCacheFileRoot(String cacheFileRoot) {
+ this.cacheFileRoot = cacheFileRoot;
+ }
+
+ public void setBatchSize(long batchSize) {
+ this.recommendedBatchSize = batchSize;
--- End diff --
Would it be good to add an assert to check recommendedBatchSize <= PARQUET_READER_RECORD_BATCH_SIZE max_value?
---
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 #597: DRILL-4905: Push down the LIMIT to the parquet read...
Posted by jinfengni <gi...@git.apache.org>.
Github user jinfengni commented on a diff in the pull request:
https://github.com/apache/drill/pull/597#discussion_r80608344
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/ParquetScanBatchCreator.java ---
@@ -107,7 +107,7 @@ public ScanBatch getBatch(FragmentContext context, ParquetRowGroupScan rowGroupS
if (!context.getOptions().getOption(ExecConstants.PARQUET_NEW_RECORD_READER).bool_val && !isComplex(footers.get(e.getPath()))) {
readers.add(
new ParquetRecordReader(
- context, e.getPath(), e.getRowGroupIndex(), fs,
+ context, rowGroupScan.getBatchSize(), e.getPath(), e.getRowGroupIndex(), fs,
--- End diff --
What about DrillParquetReader?
---
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.
---
Re: [GitHub] drill issue #597: DRILL-4905: Push the LIMIT down to the parquet reader.
Posted by Padma Penumarthy <pp...@maprtech.com>.
Thanks for the review.
Thanks,
Padma
> On Oct 21, 2016, at 2:16 PM, jinfengni <gi...@git.apache.org> wrote:
>
> Github user jinfengni commented on the issue:
>
> https://github.com/apache/drill/pull/597
>
> +1
>
> Thanks for the PR. It's good that you added couple of more unit tests.
>
>
>
> ---
> 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 #597: DRILL-4905: Push the LIMIT down to the parquet reader.
Posted by jinfengni <gi...@git.apache.org>.
Github user jinfengni commented on the issue:
https://github.com/apache/drill/pull/597
+1
Thanks for the PR. It's good that you added couple of more unit tests.
---
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 #597: DRILL-4905: Push the LIMIT down to the parquet read...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/drill/pull/597
---
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 #597: DRILL-4905: Push down the LIMIT to the parquet read...
Posted by jinfengni <gi...@git.apache.org>.
Github user jinfengni commented on a diff in the pull request:
https://github.com/apache/drill/pull/597#discussion_r81196052
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/ParquetGroupScan.java ---
@@ -926,16 +952,22 @@ public GroupScan applyLimit(long maxRecords) {
fileNames.add(rowGroupInfo.getPath());
}
- if (fileNames.size() == fileSet.size() ) {
+ // If there is no change in fileSet and maxRecords is >= batchSize, no need to create new groupScan.
+ if (fileNames.size() == fileSet.size() && (maxRecords >= recommendedBatchSize) ) {
// There is no reduction of rowGroups. Return the original groupScan.
logger.debug("applyLimit() does not apply!");
return null;
}
+ // If limit maxRecords is less than batchSize, update batchSize to the limit size.
+ if (maxRecords < recommendedBatchSize) {
+ recommendedBatchSize = (int) maxRecords;
+ }
+
try {
FileSelection newSelection = new FileSelection(null, Lists.newArrayList(fileNames), getSelectionRoot(), cacheFileRoot, false);
logger.debug("applyLimit() reduce parquet file # from {} to {}", fileSet.size(), fileNames.size());
- return this.clone(newSelection);
+ return this.clone(newSelection, recommendedBatchSize);
--- End diff --
I feel in case file selection is unchanged and maxRecords < recommenedBatchSize, we do not have to re-create a new parquetGroupScan. In such case, all we need is to re-set batchsize. Recreate a parquetgroup with same fileselection would incur overhead of reading parquet metadata.
---
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 #597: DRILL-4905: Push down the LIMIT to the parquet read...
Posted by jinfengni <gi...@git.apache.org>.
Github user jinfengni commented on a diff in the pull request:
https://github.com/apache/drill/pull/597#discussion_r83335163
--- Diff: exec/java-exec/src/test/java/org/apache/drill/exec/store/parquet/ParquetRecordReaderTest.java ---
@@ -637,7 +637,7 @@ public void testPerformance(@Injectable final DrillbitContext bitContext,
final FileSystem fs = new CachedSingleFileSystem(fileName);
final BufferAllocator allocator = RootAllocatorFactory.newRoot(c);
for(int i = 0; i < 25; i++) {
- final ParquetRecordReader rr = new ParquetRecordReader(context, 256000, fileName, 0, fs,
+ final ParquetRecordReader rr = new ParquetRecordReader(context, 256000, -1, fileName, 0, fs,
--- End diff --
If no param for "NumRecordsToRead" means rowCount, we do not need to change 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 pull request #597: DRILL-4905: Push down the LIMIT to the parquet read...
Posted by jinfengni <gi...@git.apache.org>.
Github user jinfengni commented on a diff in the pull request:
https://github.com/apache/drill/pull/597#discussion_r80970307
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/ParquetGroupScan.java ---
@@ -115,6 +115,8 @@
private List<RowGroupInfo> rowGroupInfos;
private Metadata.ParquetTableMetadataBase parquetTableMetadata = null;
private String cacheFileRoot = null;
+ private int batchSize;
+ private static final int DEFAULT_BATCH_LENGTH = 256 * 1024;
--- End diff --
Now I guess I understand what you mean. You want to cap store.parquet.record_batch_size to 256K. And set DEFAULT_BATCH_LENGTH =256k in ParquetGroupScan. At execution time, you pick min of batch_size and value from option, which will be no greater than the option value.
If that's correct, can we remove DEFAULT_BATCH_LENGTH in ParquetGroupScan. In stead, use the batch_size specified in the new option you added for NON-LIMIT 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] drill pull request #597: DRILL-4905: Push down the LIMIT to the parquet read...
Posted by jinfengni <gi...@git.apache.org>.
Github user jinfengni commented on a diff in the pull request:
https://github.com/apache/drill/pull/597#discussion_r83335982
--- Diff: exec/java-exec/src/test/java/org/apache/drill/exec/store/parquet/TestParquetGroupScan.java ---
@@ -117,4 +119,18 @@ public void testSelectEmptyNoCache() throws Exception {
uex.getMessage()), uex.getMessage().contains(expectedMsg));
}
}
+
+ @Test
+ public void testLimit() throws Exception {
+ List<QueryDataBatch> results = testSqlWithResults(String.format("select * from cp.`parquet/limitTest.parquet` limit 1"));
--- End diff --
Also, can you add a unit test with LIMINT n , where n > # of rows in the table?
---
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 #597: DRILL-4905: Push down the LIMIT to the parquet read...
Posted by jinfengni <gi...@git.apache.org>.
Github user jinfengni commented on a diff in the pull request:
https://github.com/apache/drill/pull/597#discussion_r80968936
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/ExecConstants.java ---
@@ -125,13 +125,14 @@
String PARQUET_WRITER_ENABLE_DICTIONARY_ENCODING = "store.parquet.enable_dictionary_encoding";
OptionValidator PARQUET_WRITER_ENABLE_DICTIONARY_ENCODING_VALIDATOR = new BooleanValidator(
PARQUET_WRITER_ENABLE_DICTIONARY_ENCODING, false);
-
String PARQUET_VECTOR_FILL_THRESHOLD = "store.parquet.vector_fill_threshold";
OptionValidator PARQUET_VECTOR_FILL_THRESHOLD_VALIDATOR = new PositiveLongValidator(PARQUET_VECTOR_FILL_THRESHOLD, 99l, 85l);
String PARQUET_VECTOR_FILL_CHECK_THRESHOLD = "store.parquet.vector_fill_check_threshold";
OptionValidator PARQUET_VECTOR_FILL_CHECK_THRESHOLD_VALIDATOR = new PositiveLongValidator(PARQUET_VECTOR_FILL_CHECK_THRESHOLD, 100l, 10l);
String PARQUET_NEW_RECORD_READER = "store.parquet.use_new_reader";
OptionValidator PARQUET_RECORD_READER_IMPLEMENTATION_VALIDATOR = new BooleanValidator(PARQUET_NEW_RECORD_READER, false);
+ String PARQUET_RECORD_BATCH_SIZE = "store.parquet.record_batch_size";
--- End diff --
It would be nice to use "store.parquet.reader.record_batch_size", since this option is for reader only, not for parquet writer. We could not change the option name once it's released.
---
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 #597: DRILL-4905: Push down the LIMIT to the parquet read...
Posted by jinfengni <gi...@git.apache.org>.
Github user jinfengni commented on a diff in the pull request:
https://github.com/apache/drill/pull/597#discussion_r80606955
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/ParquetGroupScan.java ---
@@ -115,6 +115,8 @@
private List<RowGroupInfo> rowGroupInfos;
private Metadata.ParquetTableMetadataBase parquetTableMetadata = null;
private String cacheFileRoot = null;
+ private int batchSize;
+ private static final int DEFAULT_BATCH_LENGTH = 256 * 1024;
--- End diff --
why do we have this DEFAULT_BATCH_LENGTH, in addition to the new option of "store.parquet.record_batch_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 #597: DRILL-4905: Push the LIMIT down to the parquet read...
Posted by ppadma <gi...@git.apache.org>.
Github user ppadma commented on a diff in the pull request:
https://github.com/apache/drill/pull/597#discussion_r83965044
--- Diff: exec/java-exec/src/test/java/org/apache/drill/exec/store/parquet/TestParquetGroupScan.java ---
@@ -117,4 +119,18 @@ public void testSelectEmptyNoCache() throws Exception {
uex.getMessage()), uex.getMessage().contains(expectedMsg));
}
}
+
+ @Test
+ public void testLimit() throws Exception {
+ List<QueryDataBatch> results = testSqlWithResults(String.format("select * from cp.`parquet/limitTest.parquet` limit 1"));
--- 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] drill pull request #597: DRILL-4905: Push down the LIMIT to the parquet read...
Posted by jinfengni <gi...@git.apache.org>.
Github user jinfengni commented on a diff in the pull request:
https://github.com/apache/drill/pull/597#discussion_r83335915
--- Diff: exec/java-exec/src/test/java/org/apache/drill/exec/store/parquet/TestParquetGroupScan.java ---
@@ -117,4 +119,18 @@ public void testSelectEmptyNoCache() throws Exception {
uex.getMessage()), uex.getMessage().contains(expectedMsg));
}
}
+
+ @Test
+ public void testLimit() throws Exception {
+ List<QueryDataBatch> results = testSqlWithResults(String.format("select * from cp.`parquet/limitTest.parquet` limit 1"));
--- End diff --
In stead of using a new parquet file, you may consider using cp.`tpch/nation.parquet`, unless the new file has some properties that the existing files do not have. It's preferable to use existing file, since 1) reduce drill package size, 2) people is more familiar with tpch sample files; making it easier to understand the expected result.
---
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 #597: DRILL-4905: Push down the LIMIT to the parquet read...
Posted by ppadma <gi...@git.apache.org>.
Github user ppadma commented on a diff in the pull request:
https://github.com/apache/drill/pull/597#discussion_r80817027
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/ParquetGroupScan.java ---
@@ -115,6 +115,8 @@
private List<RowGroupInfo> rowGroupInfos;
private Metadata.ParquetTableMetadataBase parquetTableMetadata = null;
private String cacheFileRoot = null;
+ private int batchSize;
+ private static final int DEFAULT_BATCH_LENGTH = 256 * 1024;
--- End diff --
Max value for store.parquet.record_batch_size is 256K. So, it cannot be set to 512K. I changed the name in ParquetGroupScan/ParquetRowGroupScan to recommendedBatchSize as we discussed. Please review new diffs.
---
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 #597: DRILL-4905: Push down the LIMIT to the parquet read...
Posted by ppadma <gi...@git.apache.org>.
Github user ppadma commented on a diff in the pull request:
https://github.com/apache/drill/pull/597#discussion_r80754060
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/ParquetGroupScan.java ---
@@ -899,6 +907,16 @@ public FileGroupScan clone(FileSelection selection) throws IOException {
return newScan;
}
+ // clone to create new groupscan with new file selection and batchSize.
+ public ParquetGroupScan clone(FileSelection selection, int batchSize) throws IOException {
+ ParquetGroupScan newScan = new ParquetGroupScan(this);
+ newScan.modifyFileSelection(selection);
+ newScan.cacheFileRoot = selection.cacheFileRoot;
--- End diff --
yes, we can. will do.
---
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 #597: DRILL-4905: Push down the LIMIT to the parquet reader scan...
Posted by jinfengni <gi...@git.apache.org>.
Github user jinfengni commented on the issue:
https://github.com/apache/drill/pull/597
comment
---
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 #597: DRILL-4905: Push down the LIMIT to the parquet read...
Posted by jinfengni <gi...@git.apache.org>.
Github user jinfengni commented on a diff in the pull request:
https://github.com/apache/drill/pull/597#discussion_r80754698
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/ParquetScanBatchCreator.java ---
@@ -107,7 +107,7 @@ public ScanBatch getBatch(FragmentContext context, ParquetRowGroupScan rowGroupS
if (!context.getOptions().getOption(ExecConstants.PARQUET_NEW_RECORD_READER).bool_val && !isComplex(footers.get(e.getPath()))) {
readers.add(
new ParquetRecordReader(
- context, e.getPath(), e.getRowGroupIndex(), fs,
+ context, rowGroupScan.getBatchSize(), e.getPath(), e.getRowGroupIndex(), fs,
--- End diff --
If it's only for one type of parquet reader, please document it in the JIRA, so that people will know 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] drill pull request #597: DRILL-4905: Push down the LIMIT to the parquet read...
Posted by gparai <gi...@git.apache.org>.
Github user gparai commented on a diff in the pull request:
https://github.com/apache/drill/pull/597#discussion_r81244826
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/ParquetGroupScan.java ---
@@ -864,6 +872,14 @@ public String getDigest() {
return toString();
}
+ public void setCacheFileRoot(String cacheFileRoot) {
+ this.cacheFileRoot = cacheFileRoot;
+ }
+
+ public void setBatchSize(long batchSize) {
+ this.recommendedBatchSize = batchSize;
--- End diff --
Yes, sure.
---
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 #597: DRILL-4905: Push down the LIMIT to the parquet reader scan...
Posted by ppadma <gi...@git.apache.org>.
Github user ppadma commented on the issue:
https://github.com/apache/drill/pull/597
updated with review 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] drill pull request #597: DRILL-4905: Push down the LIMIT to the parquet read...
Posted by jinfengni <gi...@git.apache.org>.
Github user jinfengni commented on a diff in the pull request:
https://github.com/apache/drill/pull/597#discussion_r81245030
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/ParquetGroupScan.java ---
@@ -864,6 +872,14 @@ public String getDigest() {
return toString();
}
+ public void setCacheFileRoot(String cacheFileRoot) {
+ this.cacheFileRoot = cacheFileRoot;
+ }
+
+ public void setBatchSize(long batchSize) {
+ this.recommendedBatchSize = batchSize;
--- End diff --
I feel it's fine we do not have such check. Max value for option is checked when users tries "alter session set" command.
For "limit n" query where n is > max value, ParquetRecordReader would pick min of two values, essentially ignore this recommendedBatchSize (?).
---
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 #597: DRILL-4905: Push down the LIMIT to the parquet read...
Posted by jinfengni <gi...@git.apache.org>.
Github user jinfengni commented on a diff in the pull request:
https://github.com/apache/drill/pull/597#discussion_r80754417
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/ParquetGroupScan.java ---
@@ -115,6 +115,8 @@
private List<RowGroupInfo> rowGroupInfos;
private Metadata.ParquetTableMetadataBase parquetTableMetadata = null;
private String cacheFileRoot = null;
+ private int batchSize;
+ private static final int DEFAULT_BATCH_LENGTH = 256 * 1024;
--- End diff --
If it has nothing to do with the new option, why do they have the same value of 256 * 1024? Just coincidence?
Also, since a new option is introduced, if user alters this option and set it to a different value than the default, will the option be honored in parquet reader?
---
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 #597: DRILL-4905: Push down the LIMIT to the parquet reader scan...
Posted by jinfengni <gi...@git.apache.org>.
Github user jinfengni commented on the issue:
https://github.com/apache/drill/pull/597
+1
LGTM.
If possible, please run a performance workload. The patch supposedly should not impact the performance workload. But in case I might miss something, it would be good to run the performance regression to make sure.
---
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 #597: DRILL-4905: Push down the LIMIT to the parquet read...
Posted by ppadma <gi...@git.apache.org>.
Github user ppadma commented on a diff in the pull request:
https://github.com/apache/drill/pull/597#discussion_r81238087
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/ParquetGroupScan.java ---
@@ -864,6 +872,14 @@ public String getDigest() {
return toString();
}
+ public void setCacheFileRoot(String cacheFileRoot) {
+ this.cacheFileRoot = cacheFileRoot;
+ }
+
+ public void setBatchSize(long batchSize) {
+ this.recommendedBatchSize = batchSize;
--- End diff --
ok. Added the assert. Check updated diffs.
---
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 #597: DRILL-4905: Push down the LIMIT to the parquet read...
Posted by jinfengni <gi...@git.apache.org>.
Github user jinfengni commented on a diff in the pull request:
https://github.com/apache/drill/pull/597#discussion_r83339590
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/ParquetGroupScan.java ---
@@ -913,19 +928,25 @@ public GroupScan applyLimit(long maxRecords) {
long count = 0;
int index = 0;
for (RowGroupInfo rowGroupInfo : rowGroupInfos) {
- if (count < maxRecords) {
- count += rowGroupInfo.getRowCount();
+ long rowCount = rowGroupInfo.getRowCount();
--- End diff --
List rowGroupInfos is populated in init() call, when ParquetGroupScan. Here, when DrillPushLimitIntoScanRule is fired for the first time, if we reduce parquet files, and come to Line 959, we will re-populate rowGroupInfos list.
The reason that your code works as expected is that DrillPushLimitIntoScanRule is fired twice. In the second rule execution, the file # is not reduced, but the rowGroupInfos list is updated in this for loop block.
However, I think it's not optimal to fire the rule twice. Ideally, we should avoid the second firing, since supposely it does nothing (that's a separate issue). We do not want to put code and rely on the assumption that this rule will be always fired twice.
Probably, we should update RowGroupInfos after line 959, after new group scan is created, and update its RowGroupInfos.
---
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 #597: DRILL-4905: Push the LIMIT down to the parquet read...
Posted by ppadma <gi...@git.apache.org>.
Github user ppadma commented on a diff in the pull request:
https://github.com/apache/drill/pull/597#discussion_r83965017
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/columnreaders/ParquetRecordReader.java ---
@@ -139,6 +155,11 @@ public ParquetRecordReader(
this.batchSize = batchSize;
this.footer = footer;
this.fragmentContext = fragmentContext;
+ if (numRecordsToRead == DEFAULT_RECORDS_TO_READ_NOT_SPECIFIED) {
+ this.numRecordsToRead = footer.getBlocks().get(rowGroupIndex).getRowCount();
+ } else {
+ this.numRecordsToRead = numRecordsToRead;
--- End diff --
Current code handles if numRecordsToRead is not in the range. So, I am not adding additional checks 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 #597: DRILL-4905: Push down the LIMIT to the parquet reader scan...
Posted by ppadma <gi...@git.apache.org>.
Github user ppadma commented on the issue:
https://github.com/apache/drill/pull/597
updated with all review comments taken care of. 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 pull request #597: DRILL-4905: Push down the LIMIT to the parquet read...
Posted by gparai <gi...@git.apache.org>.
Github user gparai commented on a diff in the pull request:
https://github.com/apache/drill/pull/597#discussion_r81242837
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/ParquetGroupScan.java ---
@@ -864,6 +872,14 @@ public String getDigest() {
return toString();
}
+ public void setCacheFileRoot(String cacheFileRoot) {
+ this.cacheFileRoot = cacheFileRoot;
+ }
+
+ public void setBatchSize(long batchSize) {
+ this.recommendedBatchSize = batchSize;
--- End diff --
This will check against the current value of the option rather than the max value? I thought we want to ensure never to exceed the MAX value of PARQUET_READER_RECORD_BATCH_SIZE - did I misunderstand? I looked into the code and it seems like we don't have access to the max value - we would need to add new methods to expose the same OR maybe invoke the validate method?
---
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 #597: DRILL-4905: Push down the LIMIT to the parquet read...
Posted by ppadma <gi...@git.apache.org>.
Github user ppadma commented on a diff in the pull request:
https://github.com/apache/drill/pull/597#discussion_r80751210
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/ParquetScanBatchCreator.java ---
@@ -107,7 +107,7 @@ public ScanBatch getBatch(FragmentContext context, ParquetRowGroupScan rowGroupS
if (!context.getOptions().getOption(ExecConstants.PARQUET_NEW_RECORD_READER).bool_val && !isComplex(footers.get(e.getPath()))) {
readers.add(
new ParquetRecordReader(
- context, e.getPath(), e.getRowGroupIndex(), fs,
+ context, rowGroupScan.getBatchSize(), e.getPath(), e.getRowGroupIndex(), fs,
--- End diff --
This fix is done only for native parquet reader.
---
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 #597: DRILL-4905: Push the LIMIT down to the parquet reader.
Posted by ppadma <gi...@git.apache.org>.
Github user ppadma commented on the issue:
https://github.com/apache/drill/pull/597
updated diffs with review comments taken care of.
---
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 #597: DRILL-4905: Push down the LIMIT to the parquet read...
Posted by jinfengni <gi...@git.apache.org>.
Github user jinfengni commented on a diff in the pull request:
https://github.com/apache/drill/pull/597#discussion_r83334623
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/columnreaders/ParquetRecordReader.java ---
@@ -139,6 +155,11 @@ public ParquetRecordReader(
this.batchSize = batchSize;
this.footer = footer;
this.fragmentContext = fragmentContext;
+ if (numRecordsToRead == DEFAULT_RECORDS_TO_READ_NOT_SPECIFIED) {
+ this.numRecordsToRead = footer.getBlocks().get(rowGroupIndex).getRowCount();
+ } else {
+ this.numRecordsToRead = numRecordsToRead;
--- End diff --
I assume numRecordsToRead should be within range of [1, rowCount]. Do we need enforce such range? What if I pass in 0 or rowCount + 100 ?
---
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 #597: DRILL-4905: Push down the LIMIT to the parquet read...
Posted by ppadma <gi...@git.apache.org>.
Github user ppadma commented on a diff in the pull request:
https://github.com/apache/drill/pull/597#discussion_r80751189
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/ParquetGroupScan.java ---
@@ -115,6 +115,8 @@
private List<RowGroupInfo> rowGroupInfos;
private Metadata.ParquetTableMetadataBase parquetTableMetadata = null;
private String cacheFileRoot = null;
+ private int batchSize;
+ private static final int DEFAULT_BATCH_LENGTH = 256 * 1024;
--- End diff --
The default batch length is used here to compare with the limit value and decide if we want to create new groupscan. New option has nothing to do with this. For normal case, we do not touch/use the option. It is added so we can use it if we want to change it at run time for any reason.
---
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.
---