You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@orc.apache.org by GitBox <gi...@apache.org> on 2021/01/26 18:59:45 UTC
[GitHub] [orc] pavibhai opened a new pull request #635: ORC-742: LazyIO for non-filter columns
pavibhai opened a new pull request #635:
URL: https://github.com/apache/orc/pull/635
### What changes were proposed in this pull request?
Added Lazy IO for follow columns.
* Identify columns in the presence of a filter into LEAD and FOLLOW columns
* LEAD columns are read first
* FOLLOW columns are read only if the filter selects an output
* This evaluation is reset on every stripe change
* RecordReaderImpl.nextBatch performs read until a batch has value or the file is exhaused instead of returning empty batches as was the case previously
* IO of FOLLOW columns happens the same as partial RowGroup selections during read
* In the presence of filters batches respected row group boundaries
* Filter is now defined as Consumer<FilterContext> instead of Consumer<VectorizedRowBatch>
### Why are the changes needed?
* The code changes allow for a lazy evaluation of FOLLOW columns, which in the case of reads with minimal hits gives substantial savings both of IO and CPU.
* The filter is changed to Consumer<FilterContext> to offer a convenience method on retrieving a ColumnVector using a name `FilterContext.findVector`
### How was this patch tested?
* This patch includes Unit tests that verify the IO savings accomplished as a result of this change.
* Given the interface and behavior change of the filters, some of the existing unit tests were updated to reflect the new API as well as new behavior of not reading FOLLOW columns unless required.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] dongjoon-hyun commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r565706490
##########
File path: java/core/src/java/org/apache/orc/impl/TreeReaderFactory.java
##########
@@ -316,12 +335,13 @@ protected long countNonNulls(long rows) throws IOException {
* @param batchSize Size of the column vector
* @param filterContext the information about the rows that were selected
* by the filter.
+ * @param rLevel The read level
* @throws IOException
*/
public void nextVector(ColumnVector previous,
boolean[] isNull,
final int batchSize,
- FilterContext filterContext) throws IOException {
+ FilterContext filterContext, ReadLevel rLevel) throws IOException {
Review comment:
In this case, shall we add this to the next line?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] dongjoon-hyun commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r566565876
##########
File path: java/core/src/java/org/apache/orc/impl/OrcAcidUtils.java
##########
@@ -68,7 +69,7 @@ public static long getLastFlushLength(FileSystem fs,
}
}
- private static final Charset utf8 = Charset.forName("UTF-8");
+ private static final Charset utf8 = StandardCharsets.UTF_8;
Review comment:
Please make another PR for this file change. We can merge that first.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
pavibhai commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r566076053
##########
File path: java/core/src/java/org/apache/orc/impl/TreeReaderFactory.java
##########
@@ -316,12 +335,13 @@ protected long countNonNulls(long rows) throws IOException {
* @param batchSize Size of the column vector
* @param filterContext the information about the rows that were selected
* by the filter.
+ * @param rLevel The read level
* @throws IOException
*/
public void nextVector(ColumnVector previous,
boolean[] isNull,
final int batchSize,
- FilterContext filterContext) throws IOException {
+ FilterContext filterContext, ReadLevel rLevel) throws IOException {
Review comment:
sure, changed
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
pavibhai commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r574102411
##########
File path: java/mapreduce/src/java/org/apache/orc/mapred/OrcMapredRecordReader.java
##########
@@ -98,16 +101,22 @@ public boolean next(NullWritable key, V value) throws IOException {
if (!ensureBatch()) {
return false;
}
+ int rowIdx = batch.selectedInUse ? batch.selected[rowInBatch] : rowInBatch;
if (schema.getCategory() == TypeDescription.Category.STRUCT) {
OrcStruct result = (OrcStruct) value;
List<TypeDescription> children = schema.getChildren();
int numberOfChildren = children.size();
for(int i=0; i < numberOfChildren; ++i) {
- result.setFieldValue(i, nextValue(batch.cols[i], rowInBatch,
- children.get(i), result.getFieldValue(i)));
+ TypeDescription child = children.get(i);
+ if (included == null || included[child.getId()]) {
+ result.setFieldValue(i, nextValue(batch.cols[i], rowIdx, child,
+ result.getFieldValue(i)));
+ } else {
+ result.setFieldValue(i, null);
+ }
Review comment:
Yes, we could. I need some guidance now on which changes can be combined or not.
So far we seem to have the following changes that are requested that we pull out separately:
1. Code changes like making a variable final or using the UTF8 Character Set
2. Changes to MapReduce classes
3. Findbugs exclude update
Can I create one PR that includes all of this or do you want separate PRs for each item that you requested a separate PR for?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] dongjoon-hyun commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r566565136
##########
File path: java/core/src/java/org/apache/orc/filter/FilterContext.java
##########
@@ -0,0 +1,169 @@
+/*
+ * 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.orc.filter;
+
+import org.apache.hadoop.hive.ql.exec.vector.ColumnVector;
+import org.apache.hadoop.hive.ql.exec.vector.StructColumnVector;
+import org.apache.hadoop.hive.ql.exec.vector.VectorizedRowBatch;
+import org.apache.hadoop.hive.ql.io.filter.MutableFilterContext;
+import org.apache.orc.TypeDescription;
+import org.jetbrains.annotations.NotNull;
+
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.regex.Pattern;
+
+/**
+ * This defines the input for any filter operation.
+ *
+ * It offers a convenience method for finding the column vector from a given name, that the filters
+ * can invoke to get access to the column vector.
+ */
+public class FilterContext implements MutableFilterContext {
Review comment:
This is a simple wrapper for Hive's `VectorizedRowBatch` for easy access.
Can we propose this to Hive community?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
pavibhai commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r578853891
##########
File path: java/core/src/java/org/apache/orc/impl/TreeReaderFactory.java
##########
@@ -2854,11 +2924,19 @@ public static TypeReader createTreeReader(TypeDescription readerType,
}
}
+ public static boolean isDecimalAsLong(OrcFile.Version version, int precision) {
+ return version == OrcFile.Version.UNSTABLE_PRE_2_0 &&
+ precision <= TypeDescription.MAX_DECIMAL64_PRECISION;
+ }
+
public static BatchReader createRootReader(TypeDescription readerType, Context context)
throws IOException {
TypeReader reader = createTreeReader(readerType, context);
if (reader instanceof StructTreeReader) {
- return new StructBatchReader((StructTreeReader) reader, context);
+ return new StructBatchReader(reader, context);
+ } else if (reader instanceof LevelTypeReader
+ && ((LevelTypeReader)reader).getReader() instanceof StructTreeReader) {
+ return new LevelStructBatchReader((LevelTypeReader) reader, context);
Review comment:
I understand the concern regarding doubling the type readers. I don't quite understand the suggestion on how to circumvent that.
One possibility I do see is that instead of having the conditional code within LevelTypeReader we include that into each of the Readers directly, it will be duplicated but you will have a single reference.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] dongjoon-hyun commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r565704286
##########
File path: java/core/src/java/org/apache/orc/impl/BufferChunk.java
##########
@@ -69,22 +69,8 @@ public final String toString() {
@Override
public DiskRange sliceAndShift(long offset, long end, long shiftBy) {
- assert offset <= end && offset >= this.offset && end <= this.end;
- assert offset + shiftBy >= 0;
- ByteBuffer sliceBuf = chunk.slice();
- int newPos = (int) (offset - this.offset);
- int newLimit = newPos + (int) (end - offset);
- try {
- sliceBuf.position(newPos);
- sliceBuf.limit(newLimit);
- } catch (Throwable t) {
- LOG.error("Failed to slice buffer chunk with range" + " [" + this.offset + ", " + this.end
- + "), position: " + chunk.position() + " limit: " + chunk.limit() + ", "
- + (chunk.isDirect() ? "direct" : "array") + "; to [" + offset + ", " + end + ") "
- + t.getClass());
- throw new RuntimeException(t);
- }
- return new BufferChunk(sliceBuf, offset + shiftBy);
+ // Method has no code references
+ throw new UnsupportedOperationException();
Review comment:
If this method is not used, we need to to in an independent PR.
Please revert this change from this PR.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
pavibhai commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r577087306
##########
File path: site/develop/design/lazy_filter.md
##########
@@ -0,0 +1,352 @@
+* [Lazy Filter](#LazyFilter)
+ * [Background](#Background)
+ * [Design](#Design)
+ * [SArg to Filter](#SArgtoFilter)
+ * [Read](#Read)
+ * [Configuration](#Configuration)
+ * [Tests](#Tests)
+ * [Appendix](#Appendix)
+ * [Benchmarks](#Benchmarks)
+ * [Row vs Vector](#RowvsVector)
+ * [Filter](#Filter)
+
+# Lazy Filter <a id="LazyFilter"></a>
+
+## Background <a id="Background"></a>
+
+This feature request started as a result of a large search that is performed with the following characteristics:
+
+* The search fields are not part of partition, bucket or sort fields.
+* The table is a very large table.
+* The predicates result in very few rows compared to the scan size.
+* The search columns are a significant subset of selection columns in the query.
+
+Initial analysis showed that we could have a significant benefit by lazily reading the non-search columns only when we
+have a match. We explore the design and some benchmarks in subsequent sections.
+
+## Design <a id="Design"></a>
+
+This builds further on [ORC-577][ORC-577] which currently only restricts deserialization for some selected data types
+but does not improve on IO.
+
+On a high level the design includes the following components:
+
+```text
+┌──────────────┐ ┌────────────────────────┐
+│ │ │ Read │
+│ │ │ │
+│ │ │ ┌────────────┐ │
+│SArg to Filter│─────────▶│ │Read Filter │ │
+│ │ │ │ Columns │ │
+│ │ │ └────────────┘ │
+│ │ │ │ │
+└──────────────┘ │ ▼ │
+ │ ┌────────────┐ │
+ │ │Apply Filter│ │
+ │ └────────────┘ │
+ │ │ │
+ │ ▼ │
+ │ ┌────────────┐ │
+ │ │Read Select │ │
+ │ │ Columns │ │
+ │ └────────────┘ │
+ │ │
+ │ │
+ └────────────────────────┘
+```
+
+* **SArg to Filter**: Converts Search Arguments passed down into filters for efficient application during scans.
+* **Read**: Performs the lazy read using the filters.
+ * **Read Filter Columns**: Read the filter columns from the file.
+ * **Apply Filter**: Apply the filter on the read filter columns.
+ * **Read Select Columns**: If filter selects at least a row then read the remaining columns.
+
+### SArg to Filter <a id="SArgtoFilter"></a>
+
+SArg to Filter converts the passed SArg into a filter. This enables automatic compatibility with both Spark and Hive as
+they already push down Search Arguments down to ORC.
+
+The SArg is automatically converted into a [Vector Filter][vfilter]. Which is applied during the read process. Two
+filter types were evaluated:
+
+* [Row Filter][rfilter] that evaluates each row across all the predicates once.
+* [Vector Filter][vfilter] that evaluates each filter across the entire vector and adjusts the subsequent evaluation.
+
+While a row based filter is easier to code, it is much [slower][rowvvector] to process. We also see a significant
+[performance gain][rowvvector] in the absence of normalization.
+
+The builder for search argument should allow skipping normalization during the [build][build]. This has already been
+proposed as part of [HIVE-24458][HIVE-24458].
+
+### Read <a id="Read"></a>
+
+The read process has the following changes:
+
+```text
+ │
+ │
+ │
+┌────────────────────────▼────────────────────────┐
+│ ┏━━━━━━━━━━━━━━━━┓ │
+│ ┃Plan ++Search++ ┃ │
+│ ┃ Columns ┃ │
+│ ┗━━━━━━━━━━━━━━━━┛ │
+│ Read │Stripe │
+└────────────────────────┼────────────────────────┘
+ │
+ ▼
+
+
+ │
+ │
+┌────────────────────────▼────────────────────────┐
+│ ┏━━━━━━━━━━━━━━━━┓ │
+│ ┃Read ++Search++ ┃ │
+│ ┃ Columns ┃◀─────────┐ │
+│ ┗━━━━━━━━━━━━━━━━┛ │ │
+│ │ Size = 0 │
+│ ▼ │ │
+│ ┏━━━━━━━━━━━━━━━━┓ │ │
+│ ┃ Apply Filter ┃──────────┘ │
+│ ┗━━━━━━━━━━━━━━━━┛ │
+│ Size > 0 │
+│ │ │
+│ ▼ │
+│ ┏━━━━━━━━━━━━━━━━┓ │
+│ ┃ Plan Select ┃ │
+│ ┃ Columns ┃ │
+│ ┗━━━━━━━━━━━━━━━━┛ │
+│ │ │
+│ ▼ │
+│ ┏━━━━━━━━━━━━━━━━┓ │
+│ ┃ Read Select ┃ │
+│ ┃ Columns ┃ │
+│ ┗━━━━━━━━━━━━━━━━┛ │
+│ Next │Batch │
+└────────────────────────┼────────────────────────┘
+ │
+ ▼
+```
+
+The read process changes:
+
+* **Read Stripe** used to plan the read of all (search + select) columns. This is enhanced to plan and fetch only the
+ search columns. The rest of the stripe planning process optimizations remain unchanged e.g. partial read planning of
+ the stripe based on RowGroup statistics.
+* **Next Batch** identifies the processing that takes place when `RecordReader.nextBatch` is invoked.
+ * **Read Search Columns** takes place instead of reading all the selected columns. This is in sync with the planning
+ that has taken place during **Read Stripe** where only the search columns have been planned.
+ * **Apply Filter** on the batch that at this point only includes search columns. Evaluate the result of the filter:
+ * **Size = 0** indicates all records have been filtered out. Given this we proceed to the next batch of search
+ columns.
+ * **Size > 0** indicates that at least one record accepted by the filter. This record needs to be substantiated with
+ other columns.
+ * **Plan Select Columns** is invoked to perform read of the select columns. The planning happens as follows:
+ * Determine the current position of the read within the stripe and plan the read for the select columns from this
+ point forward to the end of the stripe.
+ * The Read planning of select columns respects the row groups filtered out as a result of the stripe planning.
+ * Fetch the select columns using the above plan.
+ * **Read Select Columns** into the vectorized row batch
+ * Return this batch.
+
+The current implementation performs a single read for the select columns in a stripe.
+
+```text
+┌──────────────────────────────────────────────────┐
+│ ┌────┐ ┌────┐ ┌────┐ ┌────┐ ┌────┐ ┌────┐ ┌────┐ │
+│ │RG0 │ │RG1 │ │RG2■│ │RG3 │ │RG4 │ │RG5■│ │RG6 │ │
+│ └────┘ └────┘ └────┘ └────┘ └────┘ └────┘ └────┘ │
+│ Stripe │
+└──────────────────────────────────────────────────┘
+```
+
+The above diagram depicts a stripe with 7 Row Groups out of which **RG2** and **RG5** are selected by the filter. The
+current implementation does the following:
+
+* Start the read planning process from the first match RG2
+* Read to the end of the stripe that includes RG6
+* Based on the above fetch skips RG0 and RG1 subject to compression block boundaries
+
+The above logic could be enhanced to perform say **2 or n** reads before reading to the end of stripe. The current
+implementation allows 0 reads before reading to the end of the stripe. The value of **n** could be configurable but
+should avoid too many short reads.
+
+The read behavior changes as follows with multiple reads being allowed within a stripe for select columns:
+
+```text
+┌──────────────────────────────────────────────────┐
+│ ┌────┐ ┌────┐ ┌────┐ ┌────┐ ┌────┐ ┌────┐ ┌────┐ │
+│ │ │ │ │ │■■■■│ │■■■■│ │■■■■│ │■■■■│ │■■■■│ │
+│ └────┘ └────┘ └────┘ └────┘ └────┘ └────┘ └────┘ │
+│ Current implementation │
+└──────────────────────────────────────────────────┘
+┌──────────────────────────────────────────────────┐
+│ ┌────┐ ┌────┐ ┌────┐ ┌────┐ ┌────┐ ┌────┐ ┌────┐ │
+│ │ │ │ │ │■■■■│ │ │ │ │ │■■■■│ │■■■■│ │
+│ └────┘ └────┘ └────┘ └────┘ └────┘ └────┘ └────┘ │
+│ Allow 1 partial read │
+└──────────────────────────────────────────────────┘
+```
+
+The figure shows that we could read significantly fewer bytes by performing an additional read before reading to the end
Review comment:
In the example above the option was about allowing one partial read as a result that covered reading RG2 and then skips RG3 and RG4, starts at RG5 and reads to the end of the stripe.
If we configured allow 2 partial reads then it would not read RG6.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
pavibhai commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r566071643
##########
File path: java/core/src/java/org/apache/orc/impl/TreeReaderFactory.java
##########
@@ -67,7 +70,7 @@
Set<Integer> getColumnFilterIds();
- Consumer<VectorizedRowBatch> getColumnFilterCallback();
+ Consumer<org.apache.orc.filter.FilterContext> getColumnFilterCallback();
Review comment:
This class also uses `org.apache.hadoop.hive.ql.io.filter.FilterContext` that is why this is fully qualified. I am happy to consider a different name e.g. `FilterInput`.
We can make this change after we have an approach on how to handle the breaking change of `Consumer<VectorizedRowBatch>` -> `Consumer<FilterContext>`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] omalley commented on pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
omalley commented on pull request #635:
URL: https://github.com/apache/orc/pull/635#issuecomment-780899790
With the update to master, I'm seeing a findbugs failure:
Dead store to readPercentage in org.apache.orc.TestRowFilteringIOSkip.filterAlternateBatches() [org.apache.orc.TestRowFilteringIOSkip] At TestRowFilteringIOSkip.java:[line 279] DLS_DEAD_LOCAL_STORE
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai commented on pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
pavibhai commented on pull request #635:
URL: https://github.com/apache/orc/pull/635#issuecomment-781792990
@dongjoon-hyun @wgtmac @pgaref @omalley
Thanks for your feedback and suggestions.
I will spend time on breaking this PR into [multiple PRs](https://github.com/apache/orc/pull/635#issuecomment-781490401) and submit.
Give me a couple of days for this change. I will try to incorporate all of the supplied feedback into them. If you have any other feedback feel free to post on this PR, I will still be tracking this PR or we can discuss further on the new PRs, whatever is convenient for you.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
pavibhai commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r578814097
##########
File path: java/core/src/java/org/apache/orc/impl/RecordReaderImpl.java
##########
@@ -1221,52 +1244,112 @@ private boolean advanceToNextRow(
@Override
public boolean nextBatch(VectorizedRowBatch batch) throws IOException {
try {
- if (rowInStripe >= rowCountInStripe) {
- currentStripe += 1;
- if (currentStripe >= stripes.size()) {
- batch.size = 0;
- return false;
+ int batchSize;
+
+ // do...while is required to handle the case where the filter eliminates all rows in the
+ // batch
+ do {
+ if (rowInStripe >= rowCountInStripe) {
+ currentStripe += 1;
+ if (currentStripe >= stripes.size()) {
+ batch.size = 0;
+ return false;
+ }
+ // Read stripe in Memory
+ readStripe();
+ followRowInStripe = rowInStripe;
}
- // Read stripe in Memory
- readStripe();
- }
- int batchSize = computeBatchSize(batch.getMaxSize());
- rowInStripe += batchSize;
- reader.setVectorColumnCount(batch.getDataColumnCount());
- reader.nextBatch(batch, batchSize);
- advanceToNextRow(reader, rowInStripe + rowBaseInStripe, true);
- // batch.size can be modified by filter so only batchSize can tell if we actually read rows
+ batchSize = computeBatchSize(batch.getMaxSize());
+ reader.setVectorColumnCount(batch.getDataColumnCount());
+ reader.nextBatch(batch, batchSize, readLevel);
+ if (readLevel == ReadLevel.LEAD && batch.size > 0) {
+ prepareFollowingStreams(rowInStripe, followRowInStripe);
Review comment:
added
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] dongjoon-hyun commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r565702367
##########
File path: .gitignore
##########
@@ -9,3 +9,4 @@ target
*.iws
.idea
.DS_Store
+*.monopic
Review comment:
I'm wondering if this is required in this PR.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
pavibhai commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r578838110
##########
File path: java/core/src/java/org/apache/orc/impl/RecordReaderImpl.java
##########
@@ -83,17 +85,21 @@
private final boolean[] fileIncluded;
private final long rowIndexStride;
private long rowInStripe = 0;
+ private long followRowInStripe = 0;
private int currentStripe = -1;
private long rowBaseInStripe = 0;
private long rowCountInStripe = 0;
private final BatchReader reader;
private final OrcIndex indexes;
+ private final boolean[] rowIndexCols;
Review comment:
sure, renamed
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai commented on pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
pavibhai commented on pull request #635:
URL: https://github.com/apache/orc/pull/635#issuecomment-780918832
> With the update to master, I'm seeing a findbugs failure:
>
> Dead store to readPercentage in org.apache.orc.TestRowFilteringIOSkip.filterAlternateBatches() [org.apache.orc.TestRowFilteringIOSkip] At TestRowFilteringIOSkip.java:[line 279] DLS_DEAD_LOCAL_STORE
thanks fixed this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] dongjoon-hyun commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r566567127
##########
File path: java/core/src/java/org/apache/orc/impl/reader/tree/TypeReader.java
##########
@@ -28,18 +28,20 @@
public interface TypeReader {
void checkEncoding(OrcProto.ColumnEncoding encoding) throws IOException;
- void startStripe(StripePlanner planner) throws IOException;
+ void startStripe(StripePlanner planner, ReadLevel readLevel) throws IOException;
- void seek(PositionProvider[] index) throws IOException;
+ void seek(PositionProvider[] index, ReadLevel readLevel) throws IOException;
- void seek(PositionProvider index) throws IOException;
+ void seek(PositionProvider index, ReadLevel readLevel) throws IOException;
- void skipRows(long rows) throws IOException;
+ void skipRows(long rows, ReadLevel readLevel) throws IOException;
void nextVector(ColumnVector previous,
boolean[] isNull,
int batchSize,
- FilterContext filterContext) throws IOException;
+ FilterContext filterContext, ReadLevel readLevel) throws IOException;
Review comment:
This should be in a next line.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] dongjoon-hyun commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r566562936
##########
File path: java/core/src/findbugs/exclude.xml
##########
@@ -70,4 +70,8 @@
<Class name="org.apache.orc.TypeDescription"/>
<Method name="equals" />
</Match>
+ <Match>
+ <Bug pattern="URF_UNREAD_PUBLIC_OR_PROTECTED_FIELD"/>
+ <Class name="org.apache.orc.impl.reader.StripePlanner$StreamInformation"/>
+ </Match>
Review comment:
Is this relevant to this PR? Can we proceed this independently?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] dongjoon-hyun commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r566566524
##########
File path: java/core/src/java/org/apache/orc/impl/TreeReaderFactory.java
##########
@@ -663,41 +691,46 @@ public void checkEncoding(OrcProto.ColumnEncoding encoding) throws IOException {
}
@Override
- public void startStripe(StripePlanner planner) throws IOException {
- super.startStripe(planner);
+ public void startStripe(StripePlanner planner, ReadLevel readLevel) throws IOException {
+
+ super.startStripe(planner, readLevel);
StreamName name = new StreamName(columnId,
OrcProto.Stream.Kind.DATA);
reader = createIntegerReader(planner.getEncoding(columnId).getKind(),
planner.getStream(name), true, context);
}
@Override
- public void seek(PositionProvider[] index) throws IOException {
- seek(index[columnId]);
+ public void seek(PositionProvider[] index, ReadLevel readLevel) throws IOException {
+ seek(index[columnId], readLevel);
}
@Override
- public void seek(PositionProvider index) throws IOException {
- super.seek(index);
+ public void seek(PositionProvider index, ReadLevel readLevel) throws IOException {
+
+ super.seek(index, readLevel);
reader.seek(index);
}
@Override
public void nextVector(ColumnVector previousVector,
boolean[] isNull,
final int batchSize,
- FilterContext filterContext) throws IOException {
+ FilterContext filterContext,
+ ReadLevel readLevel) throws IOException {
+
final LongColumnVector result = (LongColumnVector) previousVector;
// Read present/isNull stream
- super.nextVector(result, isNull, batchSize, filterContext);
+ super.nextVector(result, isNull, batchSize, filterContext, readLevel);
// Read value entries based on isNull entries
reader.nextVector(result, result.vector, batchSize);
}
@Override
- public void skipRows(long items) throws IOException {
+ public void skipRows(long items, ReadLevel readLevel) throws IOException {
+
Review comment:
ditto. Please remove empty line insertion.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai commented on pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
pavibhai commented on pull request #635:
URL: https://github.com/apache/orc/pull/635#issuecomment-768514501
> Could your resolve conflicts by rebasing to the master branch?
done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
pavibhai commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r578854386
##########
File path: java/core/src/java/org/apache/orc/impl/reader/tree/ReadLevel.java
##########
@@ -0,0 +1,25 @@
+/*
+ * 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.orc.impl.reader.tree;
+
+public enum ReadLevel {
+ ALL, // Read every column
Review comment:
good idea, will do
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
pavibhai commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r578855075
##########
File path: java/core/src/java/org/apache/orc/impl/reader/tree/LevelTypeReader.java
##########
@@ -0,0 +1,107 @@
+/*
+ * 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.orc.impl.reader.tree;
+
+import org.apache.hadoop.hive.ql.exec.vector.ColumnVector;
+import org.apache.hadoop.hive.ql.io.filter.FilterContext;
+import org.apache.orc.OrcProto;
+import org.apache.orc.impl.PositionProvider;
+import org.apache.orc.impl.reader.StripePlanner;
+
+import java.io.IOException;
+
+/**
+ * Wrapper reader that implements the level logic.
+ * The methods are invoked on the reader only if the read level matches.
+ */
+public class LevelTypeReader implements TypeReader {
+ private final TypeReader reader;
+
+ public LevelTypeReader(TypeReader reader) {
+ this.reader = reader;
+ }
+
+ @Override
+ public void checkEncoding(OrcProto.ColumnEncoding encoding) throws IOException {
+ reader.checkEncoding(encoding);
+ }
+
+ @Override
+ public void startStripe(StripePlanner planner, ReadLevel readLevel) throws IOException {
+ if (reader.getReadLevel() != readLevel) {
+ return;
+ }
+
+ reader.startStripe(planner, readLevel);
+ }
+
+ @Override
+ public void seek(PositionProvider[] index, ReadLevel readLevel) throws IOException {
+ if (reader.getReadLevel() != readLevel) {
+ return;
+ }
+
+ reader.seek(index, readLevel);
+ }
+
+ @Override
+ public void seek(PositionProvider index, ReadLevel readLevel) throws IOException {
+ if (reader.getReadLevel() != readLevel) {
+ return;
+ }
+
+ reader.seek(index, readLevel);
+ }
+
+ @Override
+ public void skipRows(long rows, ReadLevel readLevel) throws IOException {
+ if (reader.getReadLevel() != readLevel) {
+ return;
+ }
+
+ reader.skipRows(rows, readLevel);
+ }
+
+ @Override
+ public void nextVector(ColumnVector previous,
+ boolean[] isNull,
+ int batchSize,
+ FilterContext filterContext,
+ ReadLevel readLevel) throws IOException {
+ if (reader.getReadLevel() != readLevel) {
Review comment:
will change
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pgaref edited a comment on pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
pgaref edited a comment on pull request #635:
URL: https://github.com/apache/orc/pull/635#issuecomment-781509748
> > Left some initial comments here and there but in general I will have to agree with the existing comments that this is just to hard to review this PR and needs splitting. I would propose the following split:
>
> Thanks for your feedback and suggestions @pgaref
>
> >
>
> I would like to propose updates to checkstyle based on line and indentation feedback I have received on this PR.
>
> > ```
> > * Findbugs onliner
> > ```
>
> Can you please clarify what you mean by this?
>
> > ```
> > * Changes to MapReduce classes
> >
> > * Introduce OrcFilterContext (also affects row-filter tests etc. but is manageable)
> >
> > * Introduce ReadLevel on Reader and Planner
> >
> > * Introduce [ORC-743](https://issues.apache.org/jira/browse/ORC-743) changes to the planner (partial-reads?)
> > ```
>
> [ORC-743](https://issues.apache.org/jira/browse/ORC-743) includes SArg to Filter conversion, the partial reads are part of this PR. The configuration for partial reads is absent in both the PRs right now.
>
> I will treat this as introduce partial reads, followed by [ORC-743](https://issues.apache.org/jira/browse/ORC-743) which might have other splits that it needs.
>
> > ```
> > * Documentation changes after we merge all the expected functionality
> >
> > * Code changes like making a variable final or using the UTF8 Character Set, or new methods (less critical)
> > ```
>
> Any changes that are not required we can pull it out to an unrelated PR. I would like to include methods that avoid code duplication into the PR still.
>
> > Please let me know what you think and again thanks for all the work!
>
> Hope that makes sense. Please let me know.
> > * Findbugs onliner
What I meant: https://github.com/apache/orc/pull/635/files/4f400a3893aa7d7a32ffad7970041a6e8acf7c28#diff-e1aaa921b730eea0b914cb64c8adfe6e743a0395c114d60f26ee19e036da20e9
>I will treat this as introduce partial reads,
Sounds good to me -- lets just make it configurable
> I would like to include methods that avoid code duplication into the PR still.
Sure, it makes sense
@pavibhai Sounds like a plan -- lets start splitting this and I will be happy to help along the way!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] dongjoon-hyun commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r566563608
##########
File path: java/core/src/java/org/apache/orc/Reader.java
##########
@@ -265,7 +269,7 @@ public Options schema(TypeDescription schema) {
*
* @return this
*/
- public Options setRowFilter(String[] filterColumnNames, Consumer<VectorizedRowBatch> filterCallback) {
+ public Options setRowFilter(String[] filterColumnNames, Consumer<FilterContext> filterCallback) {
Review comment:
As you know, we cannot change like this. You may try to add as an additional new function instead.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
pavibhai commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r574096594
##########
File path: java/core/src/java/org/apache/orc/Reader.java
##########
@@ -265,7 +269,7 @@ public Options schema(TypeDescription schema) {
*
* @return this
*/
- public Options setRowFilter(String[] filterColumnNames, Consumer<VectorizedRowBatch> filterCallback) {
+ public Options setRowFilter(String[] filterColumnNames, Consumer<FilterContext> filterCallback) {
Review comment:
This is not yet released in ORC so I was expecting that we should be flexible on changing this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
pavibhai commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r574099736
##########
File path: java/core/src/java/org/apache/orc/impl/reader/tree/ReadLevel.java
##########
@@ -0,0 +1,25 @@
+/*
+ * 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.orc.impl.reader.tree;
+
+public enum ReadLevel {
+ ALL, // Read every column
+ LEAD, // Read only the filter columns
+ FOLLOW // Read the non-filter columns
Review comment:
fixed
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
pavibhai commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r577090098
##########
File path: java/core/src/java/org/apache/orc/impl/reader/tree/StructBatchReader.java
##########
@@ -15,62 +15,80 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
+
package org.apache.orc.impl.reader.tree;
import org.apache.hadoop.hive.ql.exec.vector.ColumnVector;
import org.apache.hadoop.hive.ql.exec.vector.VectorizedRowBatch;
+import org.apache.orc.filter.OrcFilterContext;
import org.apache.orc.impl.TreeReaderFactory;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
import java.io.IOException;
-import java.util.Set;
public class StructBatchReader extends BatchReader {
+ private static final Logger LOG = LoggerFactory.getLogger(StructBatchReader.class);
// The reader context including row-filtering details
private final TreeReaderFactory.Context context;
+ private final TreeReaderFactory.StructTreeReader structReader;
+ private final OrcFilterContext fc;
- public StructBatchReader(TreeReaderFactory.StructTreeReader rowReader, TreeReaderFactory.Context context) {
+ public StructBatchReader(TypeReader rowReader, TreeReaderFactory.Context context) {
super(rowReader);
this.context = context;
+ this.fc = new OrcFilterContext(context.getSchemaEvolution().getReaderSchema());
+ if (rowReader instanceof TreeReaderFactory.StructTreeReader) {
+ structReader = (TreeReaderFactory.StructTreeReader) rowReader;
+ } else {
+ structReader = (TreeReaderFactory.StructTreeReader) ((LevelTypeReader)rowReader).getReader();
+ }
}
- private void readBatchColumn(VectorizedRowBatch batch, TypeReader[] children, int batchSize, int index)
- throws IOException {
+ private void readBatchColumn(VectorizedRowBatch batch,
+ TypeReader[] children,
+ int batchSize,
+ int index,
+ ReadLevel readLevel)
+ throws IOException {
ColumnVector colVector = batch.cols[index];
if (colVector != null) {
colVector.reset();
colVector.ensureSize(batchSize, false);
- children[index].nextVector(colVector, null, batchSize, batch);
+ children[index].nextVector(colVector, null, batchSize, batch, readLevel);
}
}
@Override
- public void nextBatch(VectorizedRowBatch batch, int batchSize) throws IOException {
- TypeReader[] children = ((TreeReaderFactory.StructTreeReader) rootType).fields;
- // Early expand fields --> apply filter --> expand remaining fields
- Set<Integer> earlyExpandCols = context.getColumnFilterIds();
+ public void nextBatch(VectorizedRowBatch batch, int batchSize, ReadLevel readLevel)
+ throws IOException {
+ nextBatchLevel(batch, batchSize, readLevel);
- // Clear selected and early expand columns used in Filter
- batch.selectedInUse = false;
- for (int i = 0; i < children.length && !earlyExpandCols.isEmpty() &&
- (vectorColumnCount == -1 || i < vectorColumnCount); ++i) {
- if (earlyExpandCols.contains(children[i].getColumnId())) {
- readBatchColumn(batch, children, batchSize, i);
+ if (readLevel == ReadLevel.LEAD) {
+ // Apply filter callback to reduce number of # rows selected for decoding in the next
+ // TreeReaders
+ if (this.context.getColumnFilterCallback() != null) {
+ this.context.getColumnFilterCallback().accept(fc.setBatch(batch));
}
}
- // Since we are going to filter rows based on some column values set batch.size earlier here
- batch.size = batchSize;
+ }
+
+ private void nextBatchLevel(VectorizedRowBatch batch, int batchSize, ReadLevel readLevel) throws IOException {
+ TypeReader[] children = structReader.fields;
- // Apply filter callback to reduce number of # rows selected for decoding in the next TreeReaders
- if (!earlyExpandCols.isEmpty() && this.context.getColumnFilterCallback() != null) {
- this.context.getColumnFilterCallback().accept(batch);
+ if (readLevel != ReadLevel.FOLLOW) {
+ // In case of FOLLOW we leave the selectedInUse untouched.
+ batch.selectedInUse = false;
Review comment:
This is coming from the VectorizedRowBatch, it indicates a partial selection in the batch which requires the use of the selected vector to determine which rows are valid.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
pavibhai commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r578772205
##########
File path: site/develop/design/lazy_filter.md
##########
@@ -0,0 +1,352 @@
+* [Lazy Filter](#LazyFilter)
+ * [Background](#Background)
+ * [Design](#Design)
+ * [SArg to Filter](#SArgtoFilter)
+ * [Read](#Read)
+ * [Configuration](#Configuration)
+ * [Tests](#Tests)
+ * [Appendix](#Appendix)
+ * [Benchmarks](#Benchmarks)
+ * [Row vs Vector](#RowvsVector)
+ * [Filter](#Filter)
+
+# Lazy Filter <a id="LazyFilter"></a>
+
+## Background <a id="Background"></a>
+
+This feature request started as a result of a large search that is performed with the following characteristics:
+
+* The search fields are not part of partition, bucket or sort fields.
+* The table is a very large table.
+* The predicates result in very few rows compared to the scan size.
+* The search columns are a significant subset of selection columns in the query.
+
+Initial analysis showed that we could have a significant benefit by lazily reading the non-search columns only when we
+have a match. We explore the design and some benchmarks in subsequent sections.
+
+## Design <a id="Design"></a>
+
+This builds further on [ORC-577][ORC-577] which currently only restricts deserialization for some selected data types
+but does not improve on IO.
+
+On a high level the design includes the following components:
+
+```text
+┌──────────────┐ ┌────────────────────────┐
+│ │ │ Read │
+│ │ │ │
+│ │ │ ┌────────────┐ │
+│SArg to Filter│─────────▶│ │Read Filter │ │
+│ │ │ │ Columns │ │
+│ │ │ └────────────┘ │
+│ │ │ │ │
+└──────────────┘ │ ▼ │
+ │ ┌────────────┐ │
+ │ │Apply Filter│ │
+ │ └────────────┘ │
+ │ │ │
+ │ ▼ │
+ │ ┌────────────┐ │
+ │ │Read Select │ │
+ │ │ Columns │ │
+ │ └────────────┘ │
+ │ │
+ │ │
+ └────────────────────────┘
+```
+
+* **SArg to Filter**: Converts Search Arguments passed down into filters for efficient application during scans.
+* **Read**: Performs the lazy read using the filters.
+ * **Read Filter Columns**: Read the filter columns from the file.
+ * **Apply Filter**: Apply the filter on the read filter columns.
+ * **Read Select Columns**: If filter selects at least a row then read the remaining columns.
+
+### SArg to Filter <a id="SArgtoFilter"></a>
+
+SArg to Filter converts the passed SArg into a filter. This enables automatic compatibility with both Spark and Hive as
+they already push down Search Arguments down to ORC.
+
+The SArg is automatically converted into a [Vector Filter][vfilter]. Which is applied during the read process. Two
+filter types were evaluated:
+
+* [Row Filter][rfilter] that evaluates each row across all the predicates once.
+* [Vector Filter][vfilter] that evaluates each filter across the entire vector and adjusts the subsequent evaluation.
+
+While a row based filter is easier to code, it is much [slower][rowvvector] to process. We also see a significant
+[performance gain][rowvvector] in the absence of normalization.
+
+The builder for search argument should allow skipping normalization during the [build][build]. This has already been
+proposed as part of [HIVE-24458][HIVE-24458].
+
+### Read <a id="Read"></a>
+
+The read process has the following changes:
+
+```text
+ │
+ │
+ │
+┌────────────────────────▼────────────────────────┐
+│ ┏━━━━━━━━━━━━━━━━┓ │
+│ ┃Plan ++Search++ ┃ │
+│ ┃ Columns ┃ │
+│ ┗━━━━━━━━━━━━━━━━┛ │
+│ Read │Stripe │
+└────────────────────────┼────────────────────────┘
+ │
+ ▼
+
+
+ │
+ │
+┌────────────────────────▼────────────────────────┐
+│ ┏━━━━━━━━━━━━━━━━┓ │
+│ ┃Read ++Search++ ┃ │
+│ ┃ Columns ┃◀─────────┐ │
+│ ┗━━━━━━━━━━━━━━━━┛ │ │
+│ │ Size = 0 │
+│ ▼ │ │
+│ ┏━━━━━━━━━━━━━━━━┓ │ │
+│ ┃ Apply Filter ┃──────────┘ │
+│ ┗━━━━━━━━━━━━━━━━┛ │
+│ Size > 0 │
+│ │ │
+│ ▼ │
+│ ┏━━━━━━━━━━━━━━━━┓ │
+│ ┃ Plan Select ┃ │
+│ ┃ Columns ┃ │
+│ ┗━━━━━━━━━━━━━━━━┛ │
+│ │ │
+│ ▼ │
+│ ┏━━━━━━━━━━━━━━━━┓ │
+│ ┃ Read Select ┃ │
+│ ┃ Columns ┃ │
+│ ┗━━━━━━━━━━━━━━━━┛ │
+│ Next │Batch │
+└────────────────────────┼────────────────────────┘
+ │
+ ▼
+```
+
+The read process changes:
+
+* **Read Stripe** used to plan the read of all (search + select) columns. This is enhanced to plan and fetch only the
+ search columns. The rest of the stripe planning process optimizations remain unchanged e.g. partial read planning of
+ the stripe based on RowGroup statistics.
+* **Next Batch** identifies the processing that takes place when `RecordReader.nextBatch` is invoked.
+ * **Read Search Columns** takes place instead of reading all the selected columns. This is in sync with the planning
+ that has taken place during **Read Stripe** where only the search columns have been planned.
+ * **Apply Filter** on the batch that at this point only includes search columns. Evaluate the result of the filter:
+ * **Size = 0** indicates all records have been filtered out. Given this we proceed to the next batch of search
+ columns.
+ * **Size > 0** indicates that at least one record accepted by the filter. This record needs to be substantiated with
+ other columns.
+ * **Plan Select Columns** is invoked to perform read of the select columns. The planning happens as follows:
+ * Determine the current position of the read within the stripe and plan the read for the select columns from this
+ point forward to the end of the stripe.
+ * The Read planning of select columns respects the row groups filtered out as a result of the stripe planning.
+ * Fetch the select columns using the above plan.
+ * **Read Select Columns** into the vectorized row batch
+ * Return this batch.
+
+The current implementation performs a single read for the select columns in a stripe.
+
+```text
+┌──────────────────────────────────────────────────┐
+│ ┌────┐ ┌────┐ ┌────┐ ┌────┐ ┌────┐ ┌────┐ ┌────┐ │
+│ │RG0 │ │RG1 │ │RG2■│ │RG3 │ │RG4 │ │RG5■│ │RG6 │ │
+│ └────┘ └────┘ └────┘ └────┘ └────┘ └────┘ └────┘ │
+│ Stripe │
+└──────────────────────────────────────────────────┘
+```
+
+The above diagram depicts a stripe with 7 Row Groups out of which **RG2** and **RG5** are selected by the filter. The
+current implementation does the following:
+
+* Start the read planning process from the first match RG2
+* Read to the end of the stripe that includes RG6
+* Based on the above fetch skips RG0 and RG1 subject to compression block boundaries
+
+The above logic could be enhanced to perform say **2 or n** reads before reading to the end of stripe. The current
+implementation allows 0 reads before reading to the end of the stripe. The value of **n** could be configurable but
+should avoid too many short reads.
+
+The read behavior changes as follows with multiple reads being allowed within a stripe for select columns:
+
+```text
+┌──────────────────────────────────────────────────┐
+│ ┌────┐ ┌────┐ ┌────┐ ┌────┐ ┌────┐ ┌────┐ ┌────┐ │
+│ │ │ │ │ │■■■■│ │■■■■│ │■■■■│ │■■■■│ │■■■■│ │
+│ └────┘ └────┘ └────┘ └────┘ └────┘ └────┘ └────┘ │
+│ Current implementation │
+└──────────────────────────────────────────────────┘
+┌──────────────────────────────────────────────────┐
+│ ┌────┐ ┌────┐ ┌────┐ ┌────┐ ┌────┐ ┌────┐ ┌────┐ │
+│ │ │ │ │ │■■■■│ │ │ │ │ │■■■■│ │■■■■│ │
+│ └────┘ └────┘ └────┘ └────┘ └────┘ └────┘ └────┘ │
+│ Allow 1 partial read │
+└──────────────────────────────────────────────────┘
+```
+
+The figure shows that we could read significantly fewer bytes by performing an additional read before reading to the end
Review comment:
No partial read configuration is not implemented.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] dongjoon-hyun commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r575587806
##########
File path: java/core/src/java/org/apache/orc/impl/ReaderImpl.java
##########
@@ -74,7 +74,7 @@
protected List<OrcProto.StripeStatistics> stripeStatistics;
private final int metadataSize;
protected final List<OrcProto.Type> types;
- private TypeDescription schema;
+ private final TypeDescription schema;
Review comment:
Yes, a separate PR, please.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
pavibhai commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r574102411
##########
File path: java/mapreduce/src/java/org/apache/orc/mapred/OrcMapredRecordReader.java
##########
@@ -98,16 +101,22 @@ public boolean next(NullWritable key, V value) throws IOException {
if (!ensureBatch()) {
return false;
}
+ int rowIdx = batch.selectedInUse ? batch.selected[rowInBatch] : rowInBatch;
if (schema.getCategory() == TypeDescription.Category.STRUCT) {
OrcStruct result = (OrcStruct) value;
List<TypeDescription> children = schema.getChildren();
int numberOfChildren = children.size();
for(int i=0; i < numberOfChildren; ++i) {
- result.setFieldValue(i, nextValue(batch.cols[i], rowInBatch,
- children.get(i), result.getFieldValue(i)));
+ TypeDescription child = children.get(i);
+ if (included == null || included[child.getId()]) {
+ result.setFieldValue(i, nextValue(batch.cols[i], rowIdx, child,
+ result.getFieldValue(i)));
+ } else {
+ result.setFieldValue(i, null);
+ }
Review comment:
Yes, we could. I need some guidance now on which changes can be combined or not.
So far we seem to have the following changes that are requested that we pull out separately:
1. Code changes like making a variable final or using the UTF8 Character Set
2. Changes to MapReduce classes
3. Findbugs exclude update
Can I create one PR that includes all of this or do you want separate PRs for each item that you requested a separate PR for?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
pavibhai commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r578826252
##########
File path: java/core/src/java/org/apache/orc/filter/OrcFilterContext.java
##########
@@ -0,0 +1,169 @@
+/*
+ * 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.orc.filter;
+
+import org.apache.hadoop.hive.ql.exec.vector.ColumnVector;
+import org.apache.hadoop.hive.ql.exec.vector.StructColumnVector;
+import org.apache.hadoop.hive.ql.exec.vector.VectorizedRowBatch;
+import org.apache.hadoop.hive.ql.io.filter.MutableFilterContext;
+import org.apache.orc.TypeDescription;
+import org.jetbrains.annotations.NotNull;
+
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.regex.Pattern;
+
+/**
+ * This defines the input for any filter operation.
+ * <p>
+ * It offers a convenience method for finding the column vector from a given name, that the filters
+ * can invoke to get access to the column vector.
+ */
+public class OrcFilterContext implements MutableFilterContext {
+
+ VectorizedRowBatch batch = null;
+ // Cache of field to ColumnVector, this is reset everytime the batch reference changes
+ private final Map<String, ColumnVector> vectors;
+ private final TypeDescription readSchema;
+
+ public OrcFilterContext(TypeDescription readSchema) {
+ this.readSchema = readSchema;
+ this.vectors = new HashMap<>();
+ }
+
+ public OrcFilterContext setBatch(@NotNull VectorizedRowBatch batch) {
+ if (batch != this.batch) {
+ this.batch = batch;
+ vectors.clear();
+ }
+ return this;
+ }
+
+ @Override
+ public void setFilterContext(boolean selectedInUse, int[] selected, int selectedSize) {
+ batch.setFilterContext(selectedInUse, selected, selectedSize);
+ }
+
+ @Override
+ public boolean validateSelected() {
+ return batch.validateSelected();
+ }
+
+ @Override
+ public int[] updateSelected(int i) {
+ return batch.updateSelected(i);
+ }
+
+ @Override
+ public void setSelectedInUse(boolean b) {
+ batch.setSelectedInUse(b);
+ }
+
+ @Override
+ public void setSelected(int[] ints) {
+ batch.setSelected(ints);
+ }
+
+ @Override
+ public void setSelectedSize(int i) {
+ batch.setSelectedSize(i);
+ }
+
+ @Override
+ public void reset() {
+ batch.reset();
+ }
+
+ @Override
+ public boolean isSelectedInUse() {
+ return batch.isSelectedInUse();
+ }
+
+ @Override
+ public int[] getSelected() {
+ return batch.getSelected();
+ }
+
+ @Override
+ public int getSelectedSize() {
+ return batch.getSelectedSize();
+ }
+
+ public ColumnVector[] getCols() {
+ return batch.cols;
+ }
+
+ /**
+ * Retrieves the column vector that matches the specified name. Allows support for nested struct
+ * references e.g. order.date where data is a field in a struct called order.
+ *
+ * @param name The column name whose vector should be retrieved
+ * @return The column vector
+ * @throws IllegalArgumentException if the field is not found or if the nested field is not part
+ * of a struct
+ */
+ public ColumnVector findColumnVector(String name) {
+ if (!vectors.containsKey(name)) {
+ vectors.put(name, findVector(name));
+ }
+
+ return vectors.get(name);
+ }
+
+ private ColumnVector findVector(String name) {
+ String[] refs = name.split(SPLIT_ON_PERIOD);
Review comment:
Maybe I am misinterpreting your comment. `RecordReaderImpl.findColumns` gives me id, but I need index to determine the column vector in the row batch.
I can use the ParserUtils.splitName instead of custom split.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai closed pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
pavibhai closed pull request #635:
URL: https://github.com/apache/orc/pull/635
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
pavibhai commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r578853891
##########
File path: java/core/src/java/org/apache/orc/impl/TreeReaderFactory.java
##########
@@ -2854,11 +2924,19 @@ public static TypeReader createTreeReader(TypeDescription readerType,
}
}
+ public static boolean isDecimalAsLong(OrcFile.Version version, int precision) {
+ return version == OrcFile.Version.UNSTABLE_PRE_2_0 &&
+ precision <= TypeDescription.MAX_DECIMAL64_PRECISION;
+ }
+
public static BatchReader createRootReader(TypeDescription readerType, Context context)
throws IOException {
TypeReader reader = createTreeReader(readerType, context);
if (reader instanceof StructTreeReader) {
- return new StructBatchReader((StructTreeReader) reader, context);
+ return new StructBatchReader(reader, context);
+ } else if (reader instanceof LevelTypeReader
+ && ((LevelTypeReader)reader).getReader() instanceof StructTreeReader) {
+ return new LevelStructBatchReader((LevelTypeReader) reader, context);
Review comment:
I understand the concern regarding doubling the type readers. I don't quite understand the suggestion on how to circumvent that.
One possibility I do see is that instead of having the conditional code within LevelTypeReader, I could include this conditional processing into the readers, I can see if there is another way of centralizing this without duplicating the logic.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] dongjoon-hyun commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r566562692
##########
File path: java/core/src/java/org/apache/orc/impl/reader/StripePlanner.java
##########
@@ -144,10 +165,23 @@ public StripePlanner parseStripe(StripeInformation stripe,
* @return the buffers that were read
*/
public BufferChunkList readData(OrcIndex index,
- boolean[] rowGroupInclude,
- boolean forceDirect) throws IOException {
+ boolean[] rowGroupInclude,
+ boolean forceDirect) throws IOException {
+ ReadLevel readLevel = filterColIds.isEmpty() ? ReadLevel.ALL : ReadLevel.LEAD;
BufferChunkList chunks = (index == null || rowGroupInclude == null)
- ? planDataReading() : planPartialDataReading(index, rowGroupInclude);
+ ? planDataReading(readLevel) : planPartialDataReading(index, rowGroupInclude, readLevel);
+ dataReader.readFileData(chunks, forceDirect);
+ return chunks;
+ }
+
+ public BufferChunkList readFollowData(OrcIndex index,
+ boolean[] rowGroupInclude,
Review comment:
indentation?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
pavibhai commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r589647370
##########
File path: java/core/src/java/org/apache/orc/filter/OrcFilterContext.java
##########
@@ -0,0 +1,169 @@
+/*
+ * 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.orc.filter;
+
+import org.apache.hadoop.hive.ql.exec.vector.ColumnVector;
+import org.apache.hadoop.hive.ql.exec.vector.StructColumnVector;
+import org.apache.hadoop.hive.ql.exec.vector.VectorizedRowBatch;
+import org.apache.hadoop.hive.ql.io.filter.MutableFilterContext;
+import org.apache.orc.TypeDescription;
+import org.jetbrains.annotations.NotNull;
+
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.regex.Pattern;
+
+/**
+ * This defines the input for any filter operation.
+ * <p>
+ * It offers a convenience method for finding the column vector from a given name, that the filters
+ * can invoke to get access to the column vector.
+ */
+public class OrcFilterContext implements MutableFilterContext {
+
+ VectorizedRowBatch batch = null;
+ // Cache of field to ColumnVector, this is reset everytime the batch reference changes
+ private final Map<String, ColumnVector> vectors;
+ private final TypeDescription readSchema;
+
+ public OrcFilterContext(TypeDescription readSchema) {
+ this.readSchema = readSchema;
+ this.vectors = new HashMap<>();
+ }
+
+ public OrcFilterContext setBatch(@NotNull VectorizedRowBatch batch) {
+ if (batch != this.batch) {
+ this.batch = batch;
+ vectors.clear();
+ }
+ return this;
+ }
+
+ @Override
+ public void setFilterContext(boolean selectedInUse, int[] selected, int selectedSize) {
+ batch.setFilterContext(selectedInUse, selected, selectedSize);
+ }
+
+ @Override
+ public boolean validateSelected() {
+ return batch.validateSelected();
+ }
+
+ @Override
+ public int[] updateSelected(int i) {
+ return batch.updateSelected(i);
+ }
+
+ @Override
+ public void setSelectedInUse(boolean b) {
+ batch.setSelectedInUse(b);
+ }
+
+ @Override
+ public void setSelected(int[] ints) {
+ batch.setSelected(ints);
+ }
+
+ @Override
+ public void setSelectedSize(int i) {
+ batch.setSelectedSize(i);
+ }
+
+ @Override
+ public void reset() {
+ batch.reset();
+ }
+
+ @Override
+ public boolean isSelectedInUse() {
+ return batch.isSelectedInUse();
+ }
+
+ @Override
+ public int[] getSelected() {
+ return batch.getSelected();
+ }
+
+ @Override
+ public int getSelectedSize() {
+ return batch.getSelectedSize();
+ }
+
+ public ColumnVector[] getCols() {
+ return batch.cols;
+ }
+
+ /**
+ * Retrieves the column vector that matches the specified name. Allows support for nested struct
+ * references e.g. order.date where data is a field in a struct called order.
+ *
+ * @param name The column name whose vector should be retrieved
+ * @return The column vector
+ * @throws IllegalArgumentException if the field is not found or if the nested field is not part
+ * of a struct
+ */
+ public ColumnVector findColumnVector(String name) {
+ if (!vectors.containsKey(name)) {
+ vectors.put(name, findVector(name));
+ }
+
+ return vectors.get(name);
+ }
+
+ private ColumnVector findVector(String name) {
+ String[] refs = name.split(SPLIT_ON_PERIOD);
Review comment:
Merged as part of ORC-755
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] dongjoon-hyun commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r566569049
##########
File path: site/develop/design/lazy_filter.md
##########
@@ -0,0 +1,352 @@
+* [Lazy Filter](#LazyFilter)
Review comment:
Thank you for adding this doc. Let's add this separately in order to focus the implementation.
Usually, the doc becomes obsolete during reviews. So, we had better focus on the document fully after code commit.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
pavibhai commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r566067117
##########
File path: java/core/src/java/org/apache/orc/impl/BufferChunk.java
##########
@@ -69,22 +69,8 @@ public final String toString() {
@Override
public DiskRange sliceAndShift(long offset, long end, long shiftBy) {
- assert offset <= end && offset >= this.offset && end <= this.end;
- assert offset + shiftBy >= 0;
- ByteBuffer sliceBuf = chunk.slice();
- int newPos = (int) (offset - this.offset);
- int newLimit = newPos + (int) (end - offset);
- try {
- sliceBuf.position(newPos);
- sliceBuf.limit(newLimit);
- } catch (Throwable t) {
- LOG.error("Failed to slice buffer chunk with range" + " [" + this.offset + ", " + this.end
- + "), position: " + chunk.position() + " limit: " + chunk.limit() + ", "
- + (chunk.isDirect() ? "direct" : "array") + "; to [" + offset + ", " + end + ") "
- + t.getClass());
- throw new RuntimeException(t);
- }
- return new BufferChunk(sliceBuf, offset + shiftBy);
+ // Method has no code references
+ throw new UnsupportedOperationException();
Review comment:
If I don't do this, I need to handle this method call from the point of LazyIO.
Given that this method is not used I was able to handle this by `UnsupportedOperationException`, if you prefer I can give a separate PR that should be merged in advance of this PR.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
pavibhai commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r578826320
##########
File path: java/core/src/java/org/apache/orc/filter/OrcFilterContext.java
##########
@@ -0,0 +1,169 @@
+/*
+ * 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.orc.filter;
+
+import org.apache.hadoop.hive.ql.exec.vector.ColumnVector;
+import org.apache.hadoop.hive.ql.exec.vector.StructColumnVector;
+import org.apache.hadoop.hive.ql.exec.vector.VectorizedRowBatch;
+import org.apache.hadoop.hive.ql.io.filter.MutableFilterContext;
+import org.apache.orc.TypeDescription;
+import org.jetbrains.annotations.NotNull;
+
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.regex.Pattern;
+
+/**
+ * This defines the input for any filter operation.
+ * <p>
+ * It offers a convenience method for finding the column vector from a given name, that the filters
+ * can invoke to get access to the column vector.
+ */
+public class OrcFilterContext implements MutableFilterContext {
Review comment:
will add to the documentation
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
pavibhai commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r578848491
##########
File path: java/core/src/java/org/apache/orc/impl/TreeReaderFactory.java
##########
@@ -254,7 +273,7 @@ protected static IntegerReader createIntegerReader(OrcProto.ColumnEncoding.Kind
}
}
- public void startStripe(StripePlanner planner) throws IOException {
+ public void startStripe(StripePlanner planner, ReadLevel readLevel) throws IOException {
Review comment:
Yes, that is how currently we trigger [startStripe for FOLLOW](https://github.com/apache/orc/pull/635/files#diff-8678415090c906d11e0aa6241ba24643192cd66617c68d8deb0f7efe74dd2ba3R1311)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] dongjoon-hyun commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r566561275
##########
File path: java/core/src/java/org/apache/orc/impl/BufferChunk.java
##########
@@ -69,22 +69,8 @@ public final String toString() {
@Override
public DiskRange sliceAndShift(long offset, long end, long shiftBy) {
- assert offset <= end && offset >= this.offset && end <= this.end;
- assert offset + shiftBy >= 0;
- ByteBuffer sliceBuf = chunk.slice();
- int newPos = (int) (offset - this.offset);
- int newLimit = newPos + (int) (end - offset);
- try {
- sliceBuf.position(newPos);
- sliceBuf.limit(newLimit);
- } catch (Throwable t) {
- LOG.error("Failed to slice buffer chunk with range" + " [" + this.offset + ", " + this.end
- + "), position: " + chunk.position() + " limit: " + chunk.limit() + ", "
- + (chunk.isDirect() ? "direct" : "array") + "; to [" + offset + ", " + end + ") "
- + t.getClass());
- throw new RuntimeException(t);
- }
- return new BufferChunk(sliceBuf, offset + shiftBy);
+ // Method has no code references
+ throw new UnsupportedOperationException();
Review comment:
Of course, we should evaluate the removal PR first.
For that PR, we need to validate with ORC/Iceberg/Hive at least.
Although there exists many downstream, currently only the above three are compatible with ORC 1.6.x.
We don't want to lose them again.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] dongjoon-hyun commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r566566425
##########
File path: java/core/src/java/org/apache/orc/impl/TreeReaderFactory.java
##########
@@ -663,41 +691,46 @@ public void checkEncoding(OrcProto.ColumnEncoding encoding) throws IOException {
}
@Override
- public void startStripe(StripePlanner planner) throws IOException {
- super.startStripe(planner);
+ public void startStripe(StripePlanner planner, ReadLevel readLevel) throws IOException {
+
+ super.startStripe(planner, readLevel);
StreamName name = new StreamName(columnId,
OrcProto.Stream.Kind.DATA);
reader = createIntegerReader(planner.getEncoding(columnId).getKind(),
planner.getStream(name), true, context);
}
@Override
- public void seek(PositionProvider[] index) throws IOException {
- seek(index[columnId]);
+ public void seek(PositionProvider[] index, ReadLevel readLevel) throws IOException {
+ seek(index[columnId], readLevel);
}
@Override
- public void seek(PositionProvider index) throws IOException {
- super.seek(index);
+ public void seek(PositionProvider index, ReadLevel readLevel) throws IOException {
+
+ super.seek(index, readLevel);
reader.seek(index);
}
@Override
public void nextVector(ColumnVector previousVector,
boolean[] isNull,
final int batchSize,
- FilterContext filterContext) throws IOException {
+ FilterContext filterContext,
+ ReadLevel readLevel) throws IOException {
+
Review comment:
Please remove all empty line insertion.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
pavibhai commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r578846430
##########
File path: java/core/src/java/org/apache/orc/Reader.java
##########
@@ -181,6 +183,8 @@
* Options for creating a RecordReader.
*/
class Options implements Cloneable {
+ private static final Logger LOG = LoggerFactory.getLogger(Options.class);
Review comment:
Will move this to #636
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
pavibhai commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r566069209
##########
File path: java/core/src/java/org/apache/orc/impl/BufferChunkList.java
##########
@@ -58,4 +66,5 @@ public void clear() {
head = null;
tail = null;
}
+
Review comment:
sure, removed this
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] dongjoon-hyun commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r566563687
##########
File path: java/core/src/java/org/apache/orc/Reader.java
##########
@@ -382,7 +386,7 @@ public SearchArgument getSearchArgument() {
return sarg;
}
- public Consumer<VectorizedRowBatch> getFilterCallback() {
+ public Consumer<FilterContext> getFilterCallback() {
Review comment:
ditto.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
pavibhai commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r566074505
##########
File path: java/core/src/java/org/apache/orc/impl/TreeReaderFactory.java
##########
@@ -195,6 +198,8 @@ public boolean fileUsedProlepticGregorian() {
protected BitFieldReader present = null;
protected final Context context;
+ protected final ReadLevel rLevel;
Review comment:
sure, changed
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai commented on pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
pavibhai commented on pull request #635:
URL: https://github.com/apache/orc/pull/635#issuecomment-806023421
I have created a new Pull Request with the changes on LazyIO #668 and am closing this. I have addressed the comments previously called out in this PR into #668 except for ones that are not applicable as a result of the changes.
We can discuss this further in #668
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
pavibhai commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r577084460
##########
File path: site/develop/design/lazy_filter.md
##########
@@ -0,0 +1,352 @@
+* [Lazy Filter](#LazyFilter)
+ * [Background](#Background)
+ * [Design](#Design)
+ * [SArg to Filter](#SArgtoFilter)
+ * [Read](#Read)
+ * [Configuration](#Configuration)
+ * [Tests](#Tests)
+ * [Appendix](#Appendix)
+ * [Benchmarks](#Benchmarks)
+ * [Row vs Vector](#RowvsVector)
+ * [Filter](#Filter)
+
+# Lazy Filter <a id="LazyFilter"></a>
+
+## Background <a id="Background"></a>
+
+This feature request started as a result of a large search that is performed with the following characteristics:
+
+* The search fields are not part of partition, bucket or sort fields.
+* The table is a very large table.
+* The predicates result in very few rows compared to the scan size.
+* The search columns are a significant subset of selection columns in the query.
+
+Initial analysis showed that we could have a significant benefit by lazily reading the non-search columns only when we
+have a match. We explore the design and some benchmarks in subsequent sections.
+
+## Design <a id="Design"></a>
+
+This builds further on [ORC-577][ORC-577] which currently only restricts deserialization for some selected data types
+but does not improve on IO.
+
+On a high level the design includes the following components:
+
+```text
+┌──────────────┐ ┌────────────────────────┐
+│ │ │ Read │
+│ │ │ │
+│ │ │ ┌────────────┐ │
+│SArg to Filter│─────────▶│ │Read Filter │ │
+│ │ │ │ Columns │ │
+│ │ │ └────────────┘ │
+│ │ │ │ │
+└──────────────┘ │ ▼ │
+ │ ┌────────────┐ │
+ │ │Apply Filter│ │
+ │ └────────────┘ │
+ │ │ │
+ │ ▼ │
+ │ ┌────────────┐ │
+ │ │Read Select │ │
+ │ │ Columns │ │
+ │ └────────────┘ │
+ │ │
+ │ │
+ └────────────────────────┘
+```
+
+* **SArg to Filter**: Converts Search Arguments passed down into filters for efficient application during scans.
+* **Read**: Performs the lazy read using the filters.
+ * **Read Filter Columns**: Read the filter columns from the file.
+ * **Apply Filter**: Apply the filter on the read filter columns.
+ * **Read Select Columns**: If filter selects at least a row then read the remaining columns.
+
+### SArg to Filter <a id="SArgtoFilter"></a>
+
+SArg to Filter converts the passed SArg into a filter. This enables automatic compatibility with both Spark and Hive as
+they already push down Search Arguments down to ORC.
+
+The SArg is automatically converted into a [Vector Filter][vfilter]. Which is applied during the read process. Two
+filter types were evaluated:
+
+* [Row Filter][rfilter] that evaluates each row across all the predicates once.
+* [Vector Filter][vfilter] that evaluates each filter across the entire vector and adjusts the subsequent evaluation.
+
+While a row based filter is easier to code, it is much [slower][rowvvector] to process. We also see a significant
+[performance gain][rowvvector] in the absence of normalization.
+
+The builder for search argument should allow skipping normalization during the [build][build]. This has already been
+proposed as part of [HIVE-24458][HIVE-24458].
+
+### Read <a id="Read"></a>
+
+The read process has the following changes:
+
+```text
+ │
+ │
+ │
+┌────────────────────────▼────────────────────────┐
+│ ┏━━━━━━━━━━━━━━━━┓ │
+│ ┃Plan ++Search++ ┃ │
+│ ┃ Columns ┃ │
+│ ┗━━━━━━━━━━━━━━━━┛ │
+│ Read │Stripe │
+└────────────────────────┼────────────────────────┘
+ │
+ ▼
+
+
+ │
+ │
+┌────────────────────────▼────────────────────────┐
+│ ┏━━━━━━━━━━━━━━━━┓ │
+│ ┃Read ++Search++ ┃ │
+│ ┃ Columns ┃◀─────────┐ │
+│ ┗━━━━━━━━━━━━━━━━┛ │ │
+│ │ Size = 0 │
+│ ▼ │ │
+│ ┏━━━━━━━━━━━━━━━━┓ │ │
+│ ┃ Apply Filter ┃──────────┘ │
+│ ┗━━━━━━━━━━━━━━━━┛ │
+│ Size > 0 │
+│ │ │
+│ ▼ │
+│ ┏━━━━━━━━━━━━━━━━┓ │
+│ ┃ Plan Select ┃ │
+│ ┃ Columns ┃ │
+│ ┗━━━━━━━━━━━━━━━━┛ │
+│ │ │
+│ ▼ │
+│ ┏━━━━━━━━━━━━━━━━┓ │
+│ ┃ Read Select ┃ │
+│ ┃ Columns ┃ │
+│ ┗━━━━━━━━━━━━━━━━┛ │
+│ Next │Batch │
+└────────────────────────┼────────────────────────┘
+ │
+ ▼
+```
+
+The read process changes:
+
+* **Read Stripe** used to plan the read of all (search + select) columns. This is enhanced to plan and fetch only the
+ search columns. The rest of the stripe planning process optimizations remain unchanged e.g. partial read planning of
+ the stripe based on RowGroup statistics.
+* **Next Batch** identifies the processing that takes place when `RecordReader.nextBatch` is invoked.
+ * **Read Search Columns** takes place instead of reading all the selected columns. This is in sync with the planning
+ that has taken place during **Read Stripe** where only the search columns have been planned.
+ * **Apply Filter** on the batch that at this point only includes search columns. Evaluate the result of the filter:
+ * **Size = 0** indicates all records have been filtered out. Given this we proceed to the next batch of search
+ columns.
+ * **Size > 0** indicates that at least one record accepted by the filter. This record needs to be substantiated with
+ other columns.
+ * **Plan Select Columns** is invoked to perform read of the select columns. The planning happens as follows:
+ * Determine the current position of the read within the stripe and plan the read for the select columns from this
+ point forward to the end of the stripe.
+ * The Read planning of select columns respects the row groups filtered out as a result of the stripe planning.
+ * Fetch the select columns using the above plan.
+ * **Read Select Columns** into the vectorized row batch
+ * Return this batch.
+
+The current implementation performs a single read for the select columns in a stripe.
+
+```text
+┌──────────────────────────────────────────────────┐
+│ ┌────┐ ┌────┐ ┌────┐ ┌────┐ ┌────┐ ┌────┐ ┌────┐ │
+│ │RG0 │ │RG1 │ │RG2■│ │RG3 │ │RG4 │ │RG5■│ │RG6 │ │
+│ └────┘ └────┘ └────┘ └────┘ └────┘ └────┘ └────┘ │
+│ Stripe │
+└──────────────────────────────────────────────────┘
+```
+
+The above diagram depicts a stripe with 7 Row Groups out of which **RG2** and **RG5** are selected by the filter. The
+current implementation does the following:
+
+* Start the read planning process from the first match RG2
+* Read to the end of the stripe that includes RG6
+* Based on the above fetch skips RG0 and RG1 subject to compression block boundaries
+
+The above logic could be enhanced to perform say **2 or n** reads before reading to the end of stripe. The current
+implementation allows 0 reads before reading to the end of the stripe. The value of **n** could be configurable but
+should avoid too many short reads.
+
+The read behavior changes as follows with multiple reads being allowed within a stripe for select columns:
+
+```text
+┌──────────────────────────────────────────────────┐
+│ ┌────┐ ┌────┐ ┌────┐ ┌────┐ ┌────┐ ┌────┐ ┌────┐ │
+│ │ │ │ │ │■■■■│ │■■■■│ │■■■■│ │■■■■│ │■■■■│ │
+│ └────┘ └────┘ └────┘ └────┘ └────┘ └────┘ └────┘ │
+│ Current implementation │
+└──────────────────────────────────────────────────┘
+┌──────────────────────────────────────────────────┐
+│ ┌────┐ ┌────┐ ┌────┐ ┌────┐ ┌────┐ ┌────┐ ┌────┐ │
+│ │ │ │ │ │■■■■│ │ │ │ │ │■■■■│ │■■■■│ │
+│ └────┘ └────┘ └────┘ └────┘ └────┘ └────┘ └────┘ │
+│ Allow 1 partial read │
+└──────────────────────────────────────────────────┘
+```
+
+The figure shows that we could read significantly fewer bytes by performing an additional read before reading to the end
+of stripe. This shall be included as a subsequent enhancement to this patch.
+
+## Configuration <a id="Configuration"></a>
+
+The following configuration options are exposed that control the filter behavior:
+
+|Property |Type |Default|
+|:--- |:--- |:--- |
+|orc.sarg.to.filter |boolean|false |
+|orc.sarg.to.filter.selected|boolean|false |
+
+* `orc.sarg.to.filter` can be used to turn off the SArg to filter conversion. This might be particularly relevant in
+ cases where the filter is expensive and does not eliminate a lot of records.
+* `orc.sarg.to.filter.selected` is an important setting that if incorrectly enabled results in wrong output. The
+ `VectorizedRowBatch` has a selected vector that defines which rows are selected. This property should be set to `true`
+ only if the consumer respects the selected vector in determining the valid rows.
+
+## Tests <a id="Tests"></a>
+
+We evaluated this patch against a search job with the following stats:
+
+* Table
+ * Size: ~**420 TB**
+ * Data fields: ~**120**
+ * Partition fields: **3**
+* Scan
+ * Search fields: 3 data fields with large (~ 1000 value) IN clauses compounded by **OR**.
+ * Select fields: 16 data fields (includes the 3 search fields), 1 partition field
+ * Search:
+ * Size: ~**180 TB**
+ * Records: **3.99 T**
+ * Selected:
+ * Size: ~**100 MB**
+ * Records: **1 M**
+
+We have observed the following reductions:
+
+|Test |IO Reduction %|CPU Reduction %|
+|:--- | ---:| ---:|
+|Same | 45| 47|
+|SELECT *| 70| 87|
+
+* The savings are more significant as you increase the number of select columns with respect to the search columns
+* When the filter selects most data, no significant penalty observed as a result of 2 IO compared with a single IO
+ * We do have a penalty as a result of the filter application on the selected records.
+
+## Appendix <a id="Appendix"></a>
+
+### Benchmarks <a id="Benchmarks"></a>
+
+#### Row vs Vector <a id="RowvsVector"></a>
+
+We start with a decision of using a Row filter vs a Vector filter. The Row filter has the advantage of simpler code vs
+the Vector filter.
+
+```bash
+java -jar target/orc-benchmarks-core-1.7.0-SNAPSHOT-uber.jar filter simple
+```
+
+|Benchmark |(fInSize)|(fType)|Mode| Cnt| Score|Error |Units|
+|:--- | ---:|:--- |:---|---:| ---:|:--- |:--- |
+|SimpleFilterBench.filter| 4|row |avgt| 20|52.260|± 0.109|us/op|
+|SimpleFilterBench.filter| 4|vector |avgt| 20|19.699|± 0.044|us/op|
+|SimpleFilterBench.filter| 8|row |avgt| 20|59.648|± 0.179|us/op|
+|SimpleFilterBench.filter| 8|vector |avgt| 20|28.655|± 0.036|us/op|
+|SimpleFilterBench.filter| 16|row |avgt| 20|56.480|± 0.190|us/op|
+|SimpleFilterBench.filter| 16|vector |avgt| 20|46.757|± 0.124|us/op|
+|SimpleFilterBench.filter| 32|row |avgt| 20|57.780|± 0.111|us/op|
+|SimpleFilterBench.filter| 32|vector |avgt| 20|52.060|± 0.333|us/op|
+|SimpleFilterBench.filter| 256|row |avgt| 20|50.898|± 0.275|us/op|
+|SimpleFilterBench.filter| 256|vector |avgt| 20|85.684|± 0.351|us/op|
+
+Explanation:
+
+* **fInSize** calls out the number of values in the IN clause.
+* **fType** calls out the whether the filter is a row based filter, or a vector based filter.
+
+Observations:
+
+* The vector based filter is significantly faster than the row based filter.
+ * At best, vector was faster by **59.62%**
+ * At worst, vector was faster by **32.14%**
+* The performance of the filters is deteriorates with the increase of the IN values, however even in this case the
+ vector filter is much better than the row filter.
+
+In the next test we use a complex filter with both AND, and OR to understand the impact of Conjunctive Normal Form on
Review comment:
The existing SArg application only happens on File, Stripe and RG statistics so even in the worst case it will RowIndexStride times better than what we see with filter.
With the filter the evaluation happens on every row.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] dongjoon-hyun commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r565704488
##########
File path: java/core/src/java/org/apache/orc/impl/BufferChunkList.java
##########
@@ -58,4 +66,5 @@ public void clear() {
head = null;
tail = null;
}
+
Review comment:
Let's not add this line.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
pavibhai commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r589640473
##########
File path: java/core/src/java/org/apache/orc/Reader.java
##########
@@ -265,7 +269,7 @@ public Options schema(TypeDescription schema) {
*
* @return this
*/
- public Options setRowFilter(String[] filterColumnNames, Consumer<VectorizedRowBatch> filterCallback) {
+ public Options setRowFilter(String[] filterColumnNames, Consumer<FilterContext> filterCallback) {
Review comment:
Merged into master as part of ORC-755
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] dongjoon-hyun commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r565705946
##########
File path: java/core/src/java/org/apache/orc/impl/TreeReaderFactory.java
##########
@@ -67,7 +70,7 @@
Set<Integer> getColumnFilterIds();
- Consumer<VectorizedRowBatch> getColumnFilterCallback();
+ Consumer<org.apache.orc.filter.FilterContext> getColumnFilterCallback();
Review comment:
Oh, we cannot import directly like `import org.apache.orc.filter.FilterContext`?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai commented on pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
pavibhai commented on pull request #635:
URL: https://github.com/apache/orc/pull/635#issuecomment-779331898
> @pavibhai . FYI, Owen seems to change `master` massively soon in the following PR.
>
> * #641
Thanks for the heads up @dongjoon-hyun
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
pavibhai commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r574098917
##########
File path: java/core/src/java/org/apache/orc/impl/ReaderImpl.java
##########
@@ -74,7 +74,7 @@
protected List<OrcProto.StripeStatistics> stripeStatistics;
private final int metadataSize;
protected final List<OrcProto.Type> types;
- private TypeDescription schema;
+ private final TypeDescription schema;
Review comment:
Any reason for revert or you prefer this also goes into a separate PR?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
pavibhai commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r600002178
##########
File path: java/core/src/java/org/apache/orc/impl/TreeReaderFactory.java
##########
@@ -2854,11 +2924,19 @@ public static TypeReader createTreeReader(TypeDescription readerType,
}
}
+ public static boolean isDecimalAsLong(OrcFile.Version version, int precision) {
+ return version == OrcFile.Version.UNSTABLE_PRE_2_0 &&
+ precision <= TypeDescription.MAX_DECIMAL64_PRECISION;
+ }
+
public static BatchReader createRootReader(TypeDescription readerType, Context context)
throws IOException {
TypeReader reader = createTreeReader(readerType, context);
if (reader instanceof StructTreeReader) {
- return new StructBatchReader((StructTreeReader) reader, context);
+ return new StructBatchReader(reader, context);
+ } else if (reader instanceof LevelTypeReader
+ && ((LevelTypeReader)reader).getReader() instanceof StructTreeReader) {
+ return new LevelStructBatchReader((LevelTypeReader) reader, context);
Review comment:
the logic has been moved into the Struct and Union TreeReaders instead.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] omalley commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
omalley commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r578836875
##########
File path: java/core/src/java/org/apache/orc/impl/TreeReaderFactory.java
##########
@@ -254,7 +273,7 @@ protected static IntegerReader createIntegerReader(OrcProto.ColumnEncoding.Kind
}
}
- public void startStripe(StripePlanner planner) throws IOException {
+ public void startStripe(StripePlanner planner, ReadLevel readLevel) throws IOException {
Review comment:
Do we really need the readLevel for start stripe? is startStripe(planner, FOLLOW) where we plan/read the next set of data?
##########
File path: java/core/src/java/org/apache/orc/Reader.java
##########
@@ -181,6 +183,8 @@
* Options for creating a RecordReader.
*/
class Options implements Cloneable {
+ private static final Logger LOG = LoggerFactory.getLogger(Options.class);
Review comment:
Is this used?
##########
File path: java/core/src/java/org/apache/orc/impl/reader/tree/ReadLevel.java
##########
@@ -0,0 +1,25 @@
+/*
+ * 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.orc.impl.reader.tree;
+
+public enum ReadLevel {
+ ALL, // Read every column
Review comment:
I think that we should remove ReadLevel.ALL and the functions like nextBatch should pass an EnumSet<ReadLevel>. Otherwise, I'm afraid that too many uses will fall through the cracks where ALL doesn't include the other values.
##########
File path: java/core/src/java/org/apache/orc/impl/RecordReaderImpl.java
##########
@@ -83,17 +85,24 @@
private final boolean[] fileIncluded;
private final long rowIndexStride;
private long rowInStripe = 0;
+ private long followRowInStripe = 0;
private int currentStripe = -1;
private long rowBaseInStripe = 0;
private long rowCountInStripe = 0;
private final BatchReader reader;
private final OrcIndex indexes;
+ // identifies the columns requiring row indexes
+ private final boolean[] rowIndexCols;
private final SargApplier sargApp;
// an array about which row groups aren't skipped
private boolean[] includedRowGroups = null;
private final DataReader dataReader;
private final int maxDiskRangeChunkLimit;
private final StripePlanner planner;
+ // identifies the type of read: FULL (no filters), LEAD (filters are present)
+ private final ReadLevel readLevel;
Review comment:
I think I'd remove this and just check whether the row level filter is null or not.
##########
File path: java/core/src/java/org/apache/orc/filter/OrcFilterContext.java
##########
@@ -0,0 +1,169 @@
+/*
+ * 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.orc.filter;
+
+import org.apache.hadoop.hive.ql.exec.vector.ColumnVector;
+import org.apache.hadoop.hive.ql.exec.vector.StructColumnVector;
+import org.apache.hadoop.hive.ql.exec.vector.VectorizedRowBatch;
+import org.apache.hadoop.hive.ql.io.filter.MutableFilterContext;
+import org.apache.orc.TypeDescription;
+import org.jetbrains.annotations.NotNull;
+
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.regex.Pattern;
+
+/**
+ * This defines the input for any filter operation.
+ * <p>
+ * It offers a convenience method for finding the column vector from a given name, that the filters
+ * can invoke to get access to the column vector.
+ */
+public class OrcFilterContext implements MutableFilterContext {
+
+ VectorizedRowBatch batch = null;
+ // Cache of field to ColumnVector, this is reset everytime the batch reference changes
+ private final Map<String, ColumnVector> vectors;
+ private final TypeDescription readSchema;
+
+ public OrcFilterContext(TypeDescription readSchema) {
+ this.readSchema = readSchema;
+ this.vectors = new HashMap<>();
+ }
+
+ public OrcFilterContext setBatch(@NotNull VectorizedRowBatch batch) {
+ if (batch != this.batch) {
+ this.batch = batch;
+ vectors.clear();
+ }
+ return this;
+ }
+
+ @Override
+ public void setFilterContext(boolean selectedInUse, int[] selected, int selectedSize) {
+ batch.setFilterContext(selectedInUse, selected, selectedSize);
+ }
+
+ @Override
+ public boolean validateSelected() {
+ return batch.validateSelected();
+ }
+
+ @Override
+ public int[] updateSelected(int i) {
+ return batch.updateSelected(i);
+ }
+
+ @Override
+ public void setSelectedInUse(boolean b) {
+ batch.setSelectedInUse(b);
+ }
+
+ @Override
+ public void setSelected(int[] ints) {
+ batch.setSelected(ints);
+ }
+
+ @Override
+ public void setSelectedSize(int i) {
+ batch.setSelectedSize(i);
+ }
+
+ @Override
+ public void reset() {
+ batch.reset();
+ }
+
+ @Override
+ public boolean isSelectedInUse() {
+ return batch.isSelectedInUse();
+ }
+
+ @Override
+ public int[] getSelected() {
+ return batch.getSelected();
+ }
+
+ @Override
+ public int getSelectedSize() {
+ return batch.getSelectedSize();
+ }
+
+ public ColumnVector[] getCols() {
+ return batch.cols;
+ }
+
+ /**
+ * Retrieves the column vector that matches the specified name. Allows support for nested struct
+ * references e.g. order.date where data is a field in a struct called order.
+ *
+ * @param name The column name whose vector should be retrieved
+ * @return The column vector
+ * @throws IllegalArgumentException if the field is not found or if the nested field is not part
+ * of a struct
+ */
+ public ColumnVector findColumnVector(String name) {
+ if (!vectors.containsKey(name)) {
+ vectors.put(name, findVector(name));
+ }
+
+ return vectors.get(name);
+ }
+
+ private ColumnVector findVector(String name) {
+ String[] refs = name.split(SPLIT_ON_PERIOD);
Review comment:
+1 with the exception that this code needs to navigate the VectorizedRowBatch/ColumnVector tree.
##########
File path: java/core/src/java/org/apache/orc/impl/BufferChunkList.java
##########
@@ -22,6 +22,14 @@
* Builds a list of buffer chunks
*/
public class BufferChunkList {
+ public BufferChunk getHead() {
Review comment:
+1
##########
File path: java/core/src/java/org/apache/orc/impl/reader/tree/LevelTypeReader.java
##########
@@ -0,0 +1,107 @@
+/*
+ * 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.orc.impl.reader.tree;
+
+import org.apache.hadoop.hive.ql.exec.vector.ColumnVector;
+import org.apache.hadoop.hive.ql.io.filter.FilterContext;
+import org.apache.orc.OrcProto;
+import org.apache.orc.impl.PositionProvider;
+import org.apache.orc.impl.reader.StripePlanner;
+
+import java.io.IOException;
+
+/**
+ * Wrapper reader that implements the level logic.
+ * The methods are invoked on the reader only if the read level matches.
+ */
+public class LevelTypeReader implements TypeReader {
+ private final TypeReader reader;
Review comment:
Since the ReadLevel is only used here, can we take the ReadLevel field and getReadLevel() and move it from the TypeReader to here?
##########
File path: java/core/src/java/org/apache/orc/filter/OrcFilterContext.java
##########
@@ -0,0 +1,169 @@
+/*
+ * 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.orc.filter;
+
+import org.apache.hadoop.hive.ql.exec.vector.ColumnVector;
+import org.apache.hadoop.hive.ql.exec.vector.StructColumnVector;
+import org.apache.hadoop.hive.ql.exec.vector.VectorizedRowBatch;
+import org.apache.hadoop.hive.ql.io.filter.MutableFilterContext;
+import org.apache.orc.TypeDescription;
+import org.jetbrains.annotations.NotNull;
+
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.regex.Pattern;
+
+/**
+ * This defines the input for any filter operation.
+ * <p>
+ * It offers a convenience method for finding the column vector from a given name, that the filters
+ * can invoke to get access to the column vector.
+ */
+public class OrcFilterContext implements MutableFilterContext {
Review comment:
I'd suggest that we make this a new interface and make a corresponding Impl class. I believe the only function that we need in the interface is ColumnVector findVector(String).
The changes to Reader should stay with the interface, which the field in ReaderImpl should become the impl class.
##########
File path: java/core/src/java/org/apache/orc/impl/reader/StripePlanner.java
##########
@@ -363,11 +396,26 @@ private void addChunk(BufferChunkList list, StreamInformation stream,
stream.firstChunk = chunk;
}
list.add(chunk);
+ stream.lastChunk = chunk;
offset += thisLen;
length -= thisLen;
}
}
+ private BufferChunkList addFollowChunk(long offset, long length) {
Review comment:
Agreed.
##########
File path: java/core/src/java/org/apache/orc/impl/reader/tree/LevelTypeReader.java
##########
@@ -0,0 +1,107 @@
+/*
+ * 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.orc.impl.reader.tree;
+
+import org.apache.hadoop.hive.ql.exec.vector.ColumnVector;
+import org.apache.hadoop.hive.ql.io.filter.FilterContext;
+import org.apache.orc.OrcProto;
+import org.apache.orc.impl.PositionProvider;
+import org.apache.orc.impl.reader.StripePlanner;
+
+import java.io.IOException;
+
+/**
+ * Wrapper reader that implements the level logic.
+ * The methods are invoked on the reader only if the read level matches.
+ */
+public class LevelTypeReader implements TypeReader {
+ private final TypeReader reader;
+
+ public LevelTypeReader(TypeReader reader) {
+ this.reader = reader;
+ }
+
+ @Override
+ public void checkEncoding(OrcProto.ColumnEncoding encoding) throws IOException {
+ reader.checkEncoding(encoding);
+ }
+
+ @Override
+ public void startStripe(StripePlanner planner, ReadLevel readLevel) throws IOException {
+ if (reader.getReadLevel() != readLevel) {
+ return;
+ }
+
+ reader.startStripe(planner, readLevel);
+ }
+
+ @Override
+ public void seek(PositionProvider[] index, ReadLevel readLevel) throws IOException {
+ if (reader.getReadLevel() != readLevel) {
+ return;
+ }
+
+ reader.seek(index, readLevel);
+ }
+
+ @Override
+ public void seek(PositionProvider index, ReadLevel readLevel) throws IOException {
+ if (reader.getReadLevel() != readLevel) {
+ return;
+ }
+
+ reader.seek(index, readLevel);
+ }
+
+ @Override
+ public void skipRows(long rows, ReadLevel readLevel) throws IOException {
+ if (reader.getReadLevel() != readLevel) {
+ return;
+ }
+
+ reader.skipRows(rows, readLevel);
+ }
+
+ @Override
+ public void nextVector(ColumnVector previous,
+ boolean[] isNull,
+ int batchSize,
+ FilterContext filterContext,
+ ReadLevel readLevel) throws IOException {
+ if (reader.getReadLevel() != readLevel) {
Review comment:
Like I wrote elsewhere, I think this parameter should be an EnumSet<ReadLevel> and the test should be readLevel.contains(reader.getLevel()).
##########
File path: java/core/src/java/org/apache/orc/impl/TreeReaderFactory.java
##########
@@ -2854,11 +2924,19 @@ public static TypeReader createTreeReader(TypeDescription readerType,
}
}
+ public static boolean isDecimalAsLong(OrcFile.Version version, int precision) {
+ return version == OrcFile.Version.UNSTABLE_PRE_2_0 &&
+ precision <= TypeDescription.MAX_DECIMAL64_PRECISION;
+ }
+
public static BatchReader createRootReader(TypeDescription readerType, Context context)
throws IOException {
TypeReader reader = createTreeReader(readerType, context);
if (reader instanceof StructTreeReader) {
- return new StructBatchReader((StructTreeReader) reader, context);
+ return new StructBatchReader(reader, context);
+ } else if (reader instanceof LevelTypeReader
+ && ((LevelTypeReader)reader).getReader() instanceof StructTreeReader) {
+ return new LevelStructBatchReader((LevelTypeReader) reader, context);
Review comment:
I'm worried that doubling the number of TypeReaders in the tree is fair amount of overhead. Could we make it just at the transitions from LEAD to FOLLOW where we insert the extra LevelTypeReaders?
##########
File path: java/core/src/java/org/apache/orc/impl/RecordReaderImpl.java
##########
@@ -83,17 +85,21 @@
private final boolean[] fileIncluded;
private final long rowIndexStride;
private long rowInStripe = 0;
+ private long followRowInStripe = 0;
private int currentStripe = -1;
private long rowBaseInStripe = 0;
private long rowCountInStripe = 0;
private final BatchReader reader;
private final OrcIndex indexes;
+ private final boolean[] rowIndexCols;
private final SargApplier sargApp;
// an array about which row groups aren't skipped
private boolean[] includedRowGroups = null;
private final DataReader dataReader;
private final int maxDiskRangeChunkLimit;
private final StripePlanner planner;
+ private final ReadLevel readLevel;
+ private boolean needFollowStripe;
Review comment:
I think this should be inverted and name haveReadFollowData.
##########
File path: java/core/src/java/org/apache/orc/filter/OrcFilterContext.java
##########
@@ -0,0 +1,169 @@
+/*
+ * 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.orc.filter;
+
+import org.apache.hadoop.hive.ql.exec.vector.ColumnVector;
+import org.apache.hadoop.hive.ql.exec.vector.StructColumnVector;
+import org.apache.hadoop.hive.ql.exec.vector.VectorizedRowBatch;
+import org.apache.hadoop.hive.ql.io.filter.MutableFilterContext;
+import org.apache.orc.TypeDescription;
+import org.jetbrains.annotations.NotNull;
+
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.regex.Pattern;
+
+/**
+ * This defines the input for any filter operation.
+ * <p>
+ * It offers a convenience method for finding the column vector from a given name, that the filters
+ * can invoke to get access to the column vector.
+ */
+public class OrcFilterContext implements MutableFilterContext {
Review comment:
Should the name be findColumnVector?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pgaref commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
pgaref commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r578392205
##########
File path: java/core/src/java/org/apache/orc/filter/OrcFilterContext.java
##########
@@ -0,0 +1,169 @@
+/*
+ * 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.orc.filter;
+
+import org.apache.hadoop.hive.ql.exec.vector.ColumnVector;
+import org.apache.hadoop.hive.ql.exec.vector.StructColumnVector;
+import org.apache.hadoop.hive.ql.exec.vector.VectorizedRowBatch;
+import org.apache.hadoop.hive.ql.io.filter.MutableFilterContext;
+import org.apache.orc.TypeDescription;
+import org.jetbrains.annotations.NotNull;
+
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.regex.Pattern;
+
+/**
+ * This defines the input for any filter operation.
+ * <p>
+ * It offers a convenience method for finding the column vector from a given name, that the filters
+ * can invoke to get access to the column vector.
+ */
+public class OrcFilterContext implements MutableFilterContext {
+
+ VectorizedRowBatch batch = null;
+ // Cache of field to ColumnVector, this is reset everytime the batch reference changes
+ private final Map<String, ColumnVector> vectors;
+ private final TypeDescription readSchema;
+
+ public OrcFilterContext(TypeDescription readSchema) {
+ this.readSchema = readSchema;
+ this.vectors = new HashMap<>();
+ }
+
+ public OrcFilterContext setBatch(@NotNull VectorizedRowBatch batch) {
+ if (batch != this.batch) {
+ this.batch = batch;
+ vectors.clear();
+ }
+ return this;
+ }
+
+ @Override
+ public void setFilterContext(boolean selectedInUse, int[] selected, int selectedSize) {
+ batch.setFilterContext(selectedInUse, selected, selectedSize);
+ }
+
+ @Override
+ public boolean validateSelected() {
+ return batch.validateSelected();
+ }
+
+ @Override
+ public int[] updateSelected(int i) {
+ return batch.updateSelected(i);
+ }
+
+ @Override
+ public void setSelectedInUse(boolean b) {
+ batch.setSelectedInUse(b);
+ }
+
+ @Override
+ public void setSelected(int[] ints) {
+ batch.setSelected(ints);
+ }
+
+ @Override
+ public void setSelectedSize(int i) {
+ batch.setSelectedSize(i);
+ }
+
+ @Override
+ public void reset() {
+ batch.reset();
+ }
+
+ @Override
+ public boolean isSelectedInUse() {
+ return batch.isSelectedInUse();
+ }
+
+ @Override
+ public int[] getSelected() {
+ return batch.getSelected();
+ }
+
+ @Override
+ public int getSelectedSize() {
+ return batch.getSelectedSize();
+ }
+
+ public ColumnVector[] getCols() {
+ return batch.cols;
+ }
+
+ /**
+ * Retrieves the column vector that matches the specified name. Allows support for nested struct
+ * references e.g. order.date where data is a field in a struct called order.
+ *
+ * @param name The column name whose vector should be retrieved
+ * @return The column vector
+ * @throws IllegalArgumentException if the field is not found or if the nested field is not part
+ * of a struct
+ */
+ public ColumnVector findColumnVector(String name) {
+ if (!vectors.containsKey(name)) {
+ vectors.put(name, findVector(name));
+ }
+
+ return vectors.get(name);
+ }
+
+ private ColumnVector findVector(String name) {
+ String[] refs = name.split(SPLIT_ON_PERIOD);
Review comment:
We already have Utility methods that can be used here, for example: ParserUtils.splitName
Actually, RecordReaderImpl.findColumns does everything you need under the hood -- you just need to do the extra check for the Struct Type
##########
File path: java/core/src/java/org/apache/orc/filter/OrcFilterContext.java
##########
@@ -0,0 +1,169 @@
+/*
+ * 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.orc.filter;
+
+import org.apache.hadoop.hive.ql.exec.vector.ColumnVector;
+import org.apache.hadoop.hive.ql.exec.vector.StructColumnVector;
+import org.apache.hadoop.hive.ql.exec.vector.VectorizedRowBatch;
+import org.apache.hadoop.hive.ql.io.filter.MutableFilterContext;
+import org.apache.orc.TypeDescription;
+import org.jetbrains.annotations.NotNull;
+
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.regex.Pattern;
+
+/**
+ * This defines the input for any filter operation.
+ * <p>
+ * It offers a convenience method for finding the column vector from a given name, that the filters
+ * can invoke to get access to the column vector.
+ */
+public class OrcFilterContext implements MutableFilterContext {
Review comment:
In fact this is an extension of a VectorizedRowBatch with schema -- we cant conveniently extend VectorizedRowBatch but we should mention the above in the doc
##########
File path: java/core/src/java/org/apache/orc/impl/TreeReaderFactory.java
##########
@@ -245,7 +263,8 @@ protected static IntegerReader createIntegerReader(OrcProto.ColumnEncoding.Kind
switch (kind) {
case DIRECT_V2:
case DICTIONARY_V2:
- return new RunLengthIntegerReaderV2(in, signed, context == null ? false : context.isSkipCorrupt());
+ return new RunLengthIntegerReaderV2(in, signed,
Review comment:
This change is also unrelated -- I would suggest having this part of the Reader changes. Will add more details on my review comment
##########
File path: java/core/src/java/org/apache/orc/impl/RecordReaderImpl.java
##########
@@ -1221,52 +1244,112 @@ private boolean advanceToNextRow(
@Override
public boolean nextBatch(VectorizedRowBatch batch) throws IOException {
try {
- if (rowInStripe >= rowCountInStripe) {
- currentStripe += 1;
- if (currentStripe >= stripes.size()) {
- batch.size = 0;
- return false;
+ int batchSize;
+
+ // do...while is required to handle the case where the filter eliminates all rows in the
+ // batch
+ do {
+ if (rowInStripe >= rowCountInStripe) {
+ currentStripe += 1;
+ if (currentStripe >= stripes.size()) {
+ batch.size = 0;
+ return false;
+ }
+ // Read stripe in Memory
+ readStripe();
+ followRowInStripe = rowInStripe;
}
- // Read stripe in Memory
- readStripe();
- }
- int batchSize = computeBatchSize(batch.getMaxSize());
- rowInStripe += batchSize;
- reader.setVectorColumnCount(batch.getDataColumnCount());
- reader.nextBatch(batch, batchSize);
- advanceToNextRow(reader, rowInStripe + rowBaseInStripe, true);
- // batch.size can be modified by filter so only batchSize can tell if we actually read rows
+ batchSize = computeBatchSize(batch.getMaxSize());
+ reader.setVectorColumnCount(batch.getDataColumnCount());
+ reader.nextBatch(batch, batchSize, readLevel);
+ if (readLevel == ReadLevel.LEAD && batch.size > 0) {
+ prepareFollowingStreams(rowInStripe, followRowInStripe);
Review comment:
comment here please
##########
File path: java/core/src/java/org/apache/orc/impl/RecordReaderImpl.java
##########
@@ -83,17 +85,21 @@
private final boolean[] fileIncluded;
private final long rowIndexStride;
private long rowInStripe = 0;
+ private long followRowInStripe = 0;
private int currentStripe = -1;
private long rowBaseInStripe = 0;
private long rowCountInStripe = 0;
private final BatchReader reader;
private final OrcIndex indexes;
+ private final boolean[] rowIndexCols;
Review comment:
This is still not 100% clear -- maybe rowIndexColsToRead?
As everything evolves around ReadLevel lets make use of it
##########
File path: java/core/src/java/org/apache/orc/impl/reader/tree/StructBatchReader.java
##########
@@ -15,62 +15,80 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
+
package org.apache.orc.impl.reader.tree;
import org.apache.hadoop.hive.ql.exec.vector.ColumnVector;
import org.apache.hadoop.hive.ql.exec.vector.VectorizedRowBatch;
+import org.apache.orc.filter.OrcFilterContext;
import org.apache.orc.impl.TreeReaderFactory;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
import java.io.IOException;
-import java.util.Set;
public class StructBatchReader extends BatchReader {
+ private static final Logger LOG = LoggerFactory.getLogger(StructBatchReader.class);
// The reader context including row-filtering details
private final TreeReaderFactory.Context context;
+ private final TreeReaderFactory.StructTreeReader structReader;
+ private final OrcFilterContext fc;
- public StructBatchReader(TreeReaderFactory.StructTreeReader rowReader, TreeReaderFactory.Context context) {
+ public StructBatchReader(TypeReader rowReader, TreeReaderFactory.Context context) {
super(rowReader);
this.context = context;
+ this.fc = new OrcFilterContext(context.getSchemaEvolution().getReaderSchema());
+ if (rowReader instanceof TreeReaderFactory.StructTreeReader) {
+ structReader = (TreeReaderFactory.StructTreeReader) rowReader;
+ } else {
+ structReader = (TreeReaderFactory.StructTreeReader) ((LevelTypeReader)rowReader).getReader();
+ }
}
- private void readBatchColumn(VectorizedRowBatch batch, TypeReader[] children, int batchSize, int index)
- throws IOException {
+ private void readBatchColumn(VectorizedRowBatch batch,
+ TypeReader[] children,
+ int batchSize,
+ int index,
+ ReadLevel readLevel)
+ throws IOException {
ColumnVector colVector = batch.cols[index];
if (colVector != null) {
colVector.reset();
colVector.ensureSize(batchSize, false);
- children[index].nextVector(colVector, null, batchSize, batch);
+ children[index].nextVector(colVector, null, batchSize, batch, readLevel);
}
}
@Override
- public void nextBatch(VectorizedRowBatch batch, int batchSize) throws IOException {
- TypeReader[] children = ((TreeReaderFactory.StructTreeReader) rootType).fields;
- // Early expand fields --> apply filter --> expand remaining fields
- Set<Integer> earlyExpandCols = context.getColumnFilterIds();
+ public void nextBatch(VectorizedRowBatch batch, int batchSize, ReadLevel readLevel)
+ throws IOException {
+ nextBatchLevel(batch, batchSize, readLevel);
- // Clear selected and early expand columns used in Filter
- batch.selectedInUse = false;
- for (int i = 0; i < children.length && !earlyExpandCols.isEmpty() &&
- (vectorColumnCount == -1 || i < vectorColumnCount); ++i) {
- if (earlyExpandCols.contains(children[i].getColumnId())) {
- readBatchColumn(batch, children, batchSize, i);
+ if (readLevel == ReadLevel.LEAD) {
+ // Apply filter callback to reduce number of # rows selected for decoding in the next
+ // TreeReaders
+ if (this.context.getColumnFilterCallback() != null) {
+ this.context.getColumnFilterCallback().accept(fc.setBatch(batch));
}
}
- // Since we are going to filter rows based on some column values set batch.size earlier here
- batch.size = batchSize;
+ }
+
+ private void nextBatchLevel(VectorizedRowBatch batch, int batchSize, ReadLevel readLevel) throws IOException {
+ TypeReader[] children = structReader.fields;
- // Apply filter callback to reduce number of # rows selected for decoding in the next TreeReaders
- if (!earlyExpandCols.isEmpty() && this.context.getColumnFilterCallback() != null) {
- this.context.getColumnFilterCallback().accept(batch);
+ if (readLevel != ReadLevel.FOLLOW) {
+ // In case of FOLLOW we leave the selectedInUse untouched.
+ batch.selectedInUse = false;
Review comment:
Is this the right place to reset batch.selectedInUse? I would still keep it as a single method
##########
File path: java/core/src/java/org/apache/orc/impl/RecordReaderImpl.java
##########
@@ -234,11 +246,17 @@ protected RecordReaderImpl(ReaderImpl fileReader,
// If the column is not present in the file then this can be ignored from read.
if (expandColId != -1) {
filterColIds.add(expandColId);
+ rowIndexCols[expandColId] = true;
}
}
LOG.info("Filter Columns: " + filterColIds);
+ this.readLevel = ReadLevel.LEAD;
+ } else {
+ this.readLevel = ReadLevel.ALL;
Review comment:
Maybe its cleaner to make ReadLevel.ALL the default init value (non-final) and then change it here to READ only when filters are enabled?
##########
File path: java/core/src/java/org/apache/orc/impl/reader/StripePlanner.java
##########
@@ -68,14 +74,15 @@
private String writerTimezone;
private long currentStripeId;
private long originalStripeId;
- private Map<StreamName, StreamInformation> streams = new HashMap<>();
+ private final Map<StreamName, StreamInformation> streams = new HashMap<>();
Review comment:
unrelated
##########
File path: java/core/src/java/org/apache/orc/impl/reader/StripePlanner.java
##########
@@ -363,11 +396,26 @@ private void addChunk(BufferChunkList list, StreamInformation stream,
stream.firstChunk = chunk;
}
list.add(chunk);
+ stream.lastChunk = chunk;
offset += thisLen;
length -= thisLen;
}
}
+ private BufferChunkList addFollowChunk(long offset, long length) {
Review comment:
Method never used?
##########
File path: java/core/src/java/org/apache/orc/impl/RecordReaderImpl.java
##########
@@ -83,17 +85,24 @@
private final boolean[] fileIncluded;
private final long rowIndexStride;
private long rowInStripe = 0;
+ private long followRowInStripe = 0;
private int currentStripe = -1;
private long rowBaseInStripe = 0;
private long rowCountInStripe = 0;
private final BatchReader reader;
private final OrcIndex indexes;
+ // identifies the columns requiring row indexes
+ private final boolean[] rowIndexCols;
private final SargApplier sargApp;
// an array about which row groups aren't skipped
private boolean[] includedRowGroups = null;
private final DataReader dataReader;
private final int maxDiskRangeChunkLimit;
private final StripePlanner planner;
+ // identifies the type of read: FULL (no filters), LEAD (filters are present)
+ private final ReadLevel readLevel;
Review comment:
Seems there is a FOLLOW type as well..
##########
File path: java/core/src/java/org/apache/orc/impl/RecordReaderImpl.java
##########
@@ -748,11 +763,11 @@ private static TruthValue checkInBloomFilter(BloomFilter bf,
TruthValue result = hasNull ? TruthValue.NO_NULL : TruthValue.NO;
if (predObj instanceof Long) {
- if (bf.testLong(((Long) predObj).longValue())) {
+ if (bf.testLong((Long) predObj)) {
Review comment:
All pred changes below seem unrelated?
##########
File path: java/core/src/java/org/apache/orc/impl/BufferChunkList.java
##########
@@ -22,6 +22,14 @@
* Builds a list of buffer chunks
*/
public class BufferChunkList {
+ public BufferChunk getHead() {
Review comment:
Are these changes needed? addFollowChunk method that used Head is never called
##########
File path: java/core/src/java/org/apache/orc/impl/RecordReaderImpl.java
##########
@@ -257,8 +275,8 @@ protected RecordReaderImpl(ReaderImpl fileReader,
new OrcProto.BloomFilterIndex[columns]);
planner = new StripePlanner(evolution.getFileSchema(), encryption,
- dataReader, writerVersion, ignoreNonUtf8BloomFilter,
- maxDiskRangeChunkLimit);
+ dataReader, writerVersion, ignoreNonUtf8BloomFilter,
Review comment:
indentation seems a bit off?
##########
File path: java/core/src/java/org/apache/orc/impl/TreeReaderFactory.java
##########
@@ -2833,8 +2904,7 @@ public static TypeReader createTreeReader(TypeDescription readerType,
case DATE:
return new DateTreeReader(fileType.getId(), context);
case DECIMAL:
- if (version == OrcFile.Version.UNSTABLE_PRE_2_0 &&
- fileType.getPrecision() <= TypeDescription.MAX_DECIMAL64_PRECISION){
+ if (isDecimalAsLong(version, fileType.getPrecision())){
Review comment:
Unrelated change..separate PR?
##########
File path: java/core/src/java/org/apache/orc/impl/reader/StripePlanner.java
##########
@@ -402,10 +450,18 @@ private BufferChunkList planIndexReading(boolean[] bloomFilterColumns) {
* data.
* @return a list of merged disk ranges to read
*/
- private BufferChunkList planDataReading() {
+ private BufferChunkList planDataReading(ReadLevel readLevel) {
BufferChunkList result = new BufferChunkList();
for(StreamInformation stream: dataStreams) {
- addChunk(result, stream, stream.offset, stream.length);
+ if (filterColIds.isEmpty()
Review comment:
indentation?
##########
File path: java/core/src/java/org/apache/orc/impl/RecordReaderImpl.java
##########
@@ -1170,6 +1189,10 @@ private void advanceStripe() throws IOException {
}
}
+ private int computeRGIdx(long rowIdx) {
Review comment:
Lets add doc here
##########
File path: java/core/src/java/org/apache/orc/impl/reader/tree/ReadLevel.java
##########
@@ -0,0 +1,25 @@
+/*
+ * 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.orc.impl.reader.tree;
+
+public enum ReadLevel {
Review comment:
Shall we make this an inner class of LevelTypeReader?
##########
File path: java/core/src/java/org/apache/orc/impl/reader/tree/LevelStructBatchReader.java
##########
@@ -0,0 +1,28 @@
+/*
+ * 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.orc.impl.reader.tree;
+
+import org.apache.orc.impl.TreeReaderFactory;
+
+public class LevelStructBatchReader extends StructBatchReader {
Review comment:
Shall we make this an inner class of StructBatchReader?
##########
File path: java/core/src/test/org/apache/orc/TestRowFilteringIOSkip.java
##########
@@ -0,0 +1,415 @@
+/*
+ * 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.orc;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.FileSystem;
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.hive.ql.exec.vector.BytesColumnVector;
+import org.apache.hadoop.hive.ql.exec.vector.DecimalColumnVector;
+import org.apache.hadoop.hive.ql.exec.vector.LongColumnVector;
+import org.apache.hadoop.hive.ql.exec.vector.VectorizedRowBatch;
+import org.apache.hadoop.hive.ql.io.sarg.PredicateLeaf;
+import org.apache.hadoop.hive.ql.io.sarg.SearchArgument;
+import org.apache.hadoop.hive.ql.io.sarg.SearchArgumentFactory;
+import org.apache.hadoop.hive.serde2.io.HiveDecimalWritable;
+import org.apache.orc.filter.OrcFilterContext;
+import org.junit.Assert;
+import org.junit.BeforeClass;
+import org.junit.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.util.HashSet;
+import java.util.Random;
+import java.util.Set;
+import java.util.function.Consumer;
+
+public class TestRowFilteringIOSkip {
+ private final static Logger LOG = LoggerFactory.getLogger(TestRowFilteringIOSkip.class);
+ private static final Path workDir = new Path(System.getProperty("test.tmp.dir",
+ "target" + File.separator + "test"
Review comment:
indentation seems off in here as well
##########
File path: java/core/src/java/org/apache/orc/impl/RecordReaderUtils.java
##########
@@ -545,7 +545,7 @@ public int hashCode() {
private long currentGeneration = 0;
- private final TreeMap<Key, ByteBuffer> getBufferTree(boolean direct) {
Review comment:
I believe its the latter
##########
File path: java/core/src/java/org/apache/orc/impl/TreeReaderFactory.java
##########
@@ -2758,34 +2818,44 @@ public void checkEncoding(OrcProto.ColumnEncoding encoding) throws IOException {
}
@Override
- public void startStripe(StripePlanner planner) throws IOException {
- super.startStripe(planner);
+ public void startStripe(StripePlanner planner, ReadLevel readLevel) throws IOException {
+ super.startStripe(planner, readLevel);
lengths = createIntegerReader(planner.getEncoding(columnId).getKind(),
planner.getStream(new StreamName(columnId,
OrcProto.Stream.Kind.LENGTH)), false, context);
if (keyReader != null) {
- keyReader.startStripe(planner);
+ keyReader.startStripe(planner, readLevel);
}
if (valueReader != null) {
- valueReader.startStripe(planner);
+ valueReader.startStripe(planner, readLevel);
}
}
@Override
- public void skipRows(long items) throws IOException {
+ public void skipRows(long items, ReadLevel readLevel) throws IOException {
items = countNonNulls(items);
long childSkip = 0;
for (long i = 0; i < items; ++i) {
childSkip += lengths.next();
}
- keyReader.skipRows(childSkip);
- valueReader.skipRows(childSkip);
+ keyReader.skipRows(childSkip, readLevel);
+ valueReader.skipRows(childSkip, readLevel);
}
}
public static TypeReader createTreeReader(TypeDescription readerType,
- Context context
- ) throws IOException {
+ Context context) throws IOException {
+ TypeReader reader = createTreeReaderInternal(readerType, context);
+ if (context.getColumnFilterCallback() == null
+ || reader instanceof NullTreeReader) {
Review comment:
indentation?
##########
File path: java/core/src/java/org/apache/orc/impl/TreeReaderFactory.java
##########
@@ -663,41 +691,46 @@ public void checkEncoding(OrcProto.ColumnEncoding encoding) throws IOException {
}
@Override
- public void startStripe(StripePlanner planner) throws IOException {
- super.startStripe(planner);
+ public void startStripe(StripePlanner planner, ReadLevel readLevel) throws IOException {
+
+ super.startStripe(planner, readLevel);
StreamName name = new StreamName(columnId,
OrcProto.Stream.Kind.DATA);
reader = createIntegerReader(planner.getEncoding(columnId).getKind(),
planner.getStream(name), true, context);
}
@Override
- public void seek(PositionProvider[] index) throws IOException {
- seek(index[columnId]);
+ public void seek(PositionProvider[] index, ReadLevel readLevel) throws IOException {
+ seek(index[columnId], readLevel);
}
@Override
- public void seek(PositionProvider index) throws IOException {
- super.seek(index);
+ public void seek(PositionProvider index, ReadLevel readLevel) throws IOException {
+
+ super.seek(index, readLevel);
reader.seek(index);
}
@Override
public void nextVector(ColumnVector previousVector,
boolean[] isNull,
final int batchSize,
- FilterContext filterContext) throws IOException {
+ FilterContext filterContext,
+ ReadLevel readLevel) throws IOException {
+
Review comment:
Totally agree -- there are empty line additions allover the PR
##########
File path: site/develop/design/lazy_filter.md
##########
@@ -0,0 +1,352 @@
+* [Lazy Filter](#LazyFilter)
+ * [Background](#Background)
+ * [Design](#Design)
+ * [SArg to Filter](#SArgtoFilter)
+ * [Read](#Read)
+ * [Configuration](#Configuration)
+ * [Tests](#Tests)
+ * [Appendix](#Appendix)
+ * [Benchmarks](#Benchmarks)
+ * [Row vs Vector](#RowvsVector)
+ * [Filter](#Filter)
+
+# Lazy Filter <a id="LazyFilter"></a>
+
+## Background <a id="Background"></a>
+
+This feature request started as a result of a large search that is performed with the following characteristics:
+
+* The search fields are not part of partition, bucket or sort fields.
+* The table is a very large table.
+* The predicates result in very few rows compared to the scan size.
+* The search columns are a significant subset of selection columns in the query.
+
+Initial analysis showed that we could have a significant benefit by lazily reading the non-search columns only when we
+have a match. We explore the design and some benchmarks in subsequent sections.
+
+## Design <a id="Design"></a>
+
+This builds further on [ORC-577][ORC-577] which currently only restricts deserialization for some selected data types
+but does not improve on IO.
+
+On a high level the design includes the following components:
+
+```text
+┌──────────────┐ ┌────────────────────────┐
+│ │ │ Read │
+│ │ │ │
+│ │ │ ┌────────────┐ │
+│SArg to Filter│─────────▶│ │Read Filter │ │
+│ │ │ │ Columns │ │
+│ │ │ └────────────┘ │
+│ │ │ │ │
+└──────────────┘ │ ▼ │
+ │ ┌────────────┐ │
+ │ │Apply Filter│ │
+ │ └────────────┘ │
+ │ │ │
+ │ ▼ │
+ │ ┌────────────┐ │
+ │ │Read Select │ │
+ │ │ Columns │ │
+ │ └────────────┘ │
+ │ │
+ │ │
+ └────────────────────────┘
+```
+
+* **SArg to Filter**: Converts Search Arguments passed down into filters for efficient application during scans.
+* **Read**: Performs the lazy read using the filters.
+ * **Read Filter Columns**: Read the filter columns from the file.
+ * **Apply Filter**: Apply the filter on the read filter columns.
+ * **Read Select Columns**: If filter selects at least a row then read the remaining columns.
+
+### SArg to Filter <a id="SArgtoFilter"></a>
+
+SArg to Filter converts the passed SArg into a filter. This enables automatic compatibility with both Spark and Hive as
+they already push down Search Arguments down to ORC.
+
+The SArg is automatically converted into a [Vector Filter][vfilter]. Which is applied during the read process. Two
+filter types were evaluated:
+
+* [Row Filter][rfilter] that evaluates each row across all the predicates once.
+* [Vector Filter][vfilter] that evaluates each filter across the entire vector and adjusts the subsequent evaluation.
+
+While a row based filter is easier to code, it is much [slower][rowvvector] to process. We also see a significant
+[performance gain][rowvvector] in the absence of normalization.
+
+The builder for search argument should allow skipping normalization during the [build][build]. This has already been
+proposed as part of [HIVE-24458][HIVE-24458].
+
+### Read <a id="Read"></a>
+
+The read process has the following changes:
+
+```text
+ │
+ │
+ │
+┌────────────────────────▼────────────────────────┐
+│ ┏━━━━━━━━━━━━━━━━┓ │
+│ ┃Plan ++Search++ ┃ │
+│ ┃ Columns ┃ │
+│ ┗━━━━━━━━━━━━━━━━┛ │
+│ Read │Stripe │
+└────────────────────────┼────────────────────────┘
+ │
+ ▼
+
+
+ │
+ │
+┌────────────────────────▼────────────────────────┐
+│ ┏━━━━━━━━━━━━━━━━┓ │
+│ ┃Read ++Search++ ┃ │
+│ ┃ Columns ┃◀─────────┐ │
+│ ┗━━━━━━━━━━━━━━━━┛ │ │
+│ │ Size = 0 │
+│ ▼ │ │
+│ ┏━━━━━━━━━━━━━━━━┓ │ │
+│ ┃ Apply Filter ┃──────────┘ │
+│ ┗━━━━━━━━━━━━━━━━┛ │
+│ Size > 0 │
+│ │ │
+│ ▼ │
+│ ┏━━━━━━━━━━━━━━━━┓ │
+│ ┃ Plan Select ┃ │
+│ ┃ Columns ┃ │
+│ ┗━━━━━━━━━━━━━━━━┛ │
+│ │ │
+│ ▼ │
+│ ┏━━━━━━━━━━━━━━━━┓ │
+│ ┃ Read Select ┃ │
+│ ┃ Columns ┃ │
+│ ┗━━━━━━━━━━━━━━━━┛ │
+│ Next │Batch │
+└────────────────────────┼────────────────────────┘
+ │
+ ▼
+```
+
+The read process changes:
+
+* **Read Stripe** used to plan the read of all (search + select) columns. This is enhanced to plan and fetch only the
+ search columns. The rest of the stripe planning process optimizations remain unchanged e.g. partial read planning of
+ the stripe based on RowGroup statistics.
+* **Next Batch** identifies the processing that takes place when `RecordReader.nextBatch` is invoked.
+ * **Read Search Columns** takes place instead of reading all the selected columns. This is in sync with the planning
+ that has taken place during **Read Stripe** where only the search columns have been planned.
+ * **Apply Filter** on the batch that at this point only includes search columns. Evaluate the result of the filter:
+ * **Size = 0** indicates all records have been filtered out. Given this we proceed to the next batch of search
+ columns.
+ * **Size > 0** indicates that at least one record accepted by the filter. This record needs to be substantiated with
+ other columns.
+ * **Plan Select Columns** is invoked to perform read of the select columns. The planning happens as follows:
+ * Determine the current position of the read within the stripe and plan the read for the select columns from this
+ point forward to the end of the stripe.
+ * The Read planning of select columns respects the row groups filtered out as a result of the stripe planning.
+ * Fetch the select columns using the above plan.
+ * **Read Select Columns** into the vectorized row batch
+ * Return this batch.
+
+The current implementation performs a single read for the select columns in a stripe.
+
+```text
+┌──────────────────────────────────────────────────┐
+│ ┌────┐ ┌────┐ ┌────┐ ┌────┐ ┌────┐ ┌────┐ ┌────┐ │
+│ │RG0 │ │RG1 │ │RG2■│ │RG3 │ │RG4 │ │RG5■│ │RG6 │ │
+│ └────┘ └────┘ └────┘ └────┘ └────┘ └────┘ └────┘ │
+│ Stripe │
+└──────────────────────────────────────────────────┘
+```
+
+The above diagram depicts a stripe with 7 Row Groups out of which **RG2** and **RG5** are selected by the filter. The
+current implementation does the following:
+
+* Start the read planning process from the first match RG2
+* Read to the end of the stripe that includes RG6
+* Based on the above fetch skips RG0 and RG1 subject to compression block boundaries
+
+The above logic could be enhanced to perform say **2 or n** reads before reading to the end of stripe. The current
+implementation allows 0 reads before reading to the end of the stripe. The value of **n** could be configurable but
+should avoid too many short reads.
+
+The read behavior changes as follows with multiple reads being allowed within a stripe for select columns:
+
+```text
+┌──────────────────────────────────────────────────┐
+│ ┌────┐ ┌────┐ ┌────┐ ┌────┐ ┌────┐ ┌────┐ ┌────┐ │
+│ │ │ │ │ │■■■■│ │■■■■│ │■■■■│ │■■■■│ │■■■■│ │
+│ └────┘ └────┘ └────┘ └────┘ └────┘ └────┘ └────┘ │
+│ Current implementation │
+└──────────────────────────────────────────────────┘
+┌──────────────────────────────────────────────────┐
+│ ┌────┐ ┌────┐ ┌────┐ ┌────┐ ┌────┐ ┌────┐ ┌────┐ │
+│ │ │ │ │ │■■■■│ │ │ │ │ │■■■■│ │■■■■│ │
+│ └────┘ └────┘ └────┘ └────┘ └────┘ └────┘ └────┘ │
+│ Allow 1 partial read │
+└──────────────────────────────────────────────────┘
+```
+
+The figure shows that we could read significantly fewer bytes by performing an additional read before reading to the end
+of stripe. This shall be included as a subsequent enhancement to this patch.
+
+## Configuration <a id="Configuration"></a>
+
+The following configuration options are exposed that control the filter behavior:
+
+|Property |Type |Default|
+|:--- |:--- |:--- |
+|orc.sarg.to.filter |boolean|false |
+|orc.sarg.to.filter.selected|boolean|false |
+
+* `orc.sarg.to.filter` can be used to turn off the SArg to filter conversion. This might be particularly relevant in
+ cases where the filter is expensive and does not eliminate a lot of records.
+* `orc.sarg.to.filter.selected` is an important setting that if incorrectly enabled results in wrong output. The
+ `VectorizedRowBatch` has a selected vector that defines which rows are selected. This property should be set to `true`
+ only if the consumer respects the selected vector in determining the valid rows.
+
+## Tests <a id="Tests"></a>
+
+We evaluated this patch against a search job with the following stats:
+
+* Table
+ * Size: ~**420 TB**
+ * Data fields: ~**120**
+ * Partition fields: **3**
+* Scan
+ * Search fields: 3 data fields with large (~ 1000 value) IN clauses compounded by **OR**.
+ * Select fields: 16 data fields (includes the 3 search fields), 1 partition field
+ * Search:
+ * Size: ~**180 TB**
+ * Records: **3.99 T**
+ * Selected:
+ * Size: ~**100 MB**
+ * Records: **1 M**
+
+We have observed the following reductions:
+
+|Test |IO Reduction %|CPU Reduction %|
+|:--- | ---:| ---:|
+|Same | 45| 47|
+|SELECT *| 70| 87|
+
+* The savings are more significant as you increase the number of select columns with respect to the search columns
+* When the filter selects most data, no significant penalty observed as a result of 2 IO compared with a single IO
+ * We do have a penalty as a result of the filter application on the selected records.
+
+## Appendix <a id="Appendix"></a>
+
+### Benchmarks <a id="Benchmarks"></a>
+
+#### Row vs Vector <a id="RowvsVector"></a>
+
+We start with a decision of using a Row filter vs a Vector filter. The Row filter has the advantage of simpler code vs
+the Vector filter.
+
+```bash
+java -jar target/orc-benchmarks-core-1.7.0-SNAPSHOT-uber.jar filter simple
+```
+
+|Benchmark |(fInSize)|(fType)|Mode| Cnt| Score|Error |Units|
+|:--- | ---:|:--- |:---|---:| ---:|:--- |:--- |
+|SimpleFilterBench.filter| 4|row |avgt| 20|52.260|± 0.109|us/op|
+|SimpleFilterBench.filter| 4|vector |avgt| 20|19.699|± 0.044|us/op|
+|SimpleFilterBench.filter| 8|row |avgt| 20|59.648|± 0.179|us/op|
+|SimpleFilterBench.filter| 8|vector |avgt| 20|28.655|± 0.036|us/op|
+|SimpleFilterBench.filter| 16|row |avgt| 20|56.480|± 0.190|us/op|
+|SimpleFilterBench.filter| 16|vector |avgt| 20|46.757|± 0.124|us/op|
+|SimpleFilterBench.filter| 32|row |avgt| 20|57.780|± 0.111|us/op|
+|SimpleFilterBench.filter| 32|vector |avgt| 20|52.060|± 0.333|us/op|
+|SimpleFilterBench.filter| 256|row |avgt| 20|50.898|± 0.275|us/op|
+|SimpleFilterBench.filter| 256|vector |avgt| 20|85.684|± 0.351|us/op|
+
+Explanation:
+
+* **fInSize** calls out the number of values in the IN clause.
+* **fType** calls out the whether the filter is a row based filter, or a vector based filter.
+
+Observations:
+
+* The vector based filter is significantly faster than the row based filter.
+ * At best, vector was faster by **59.62%**
+ * At worst, vector was faster by **32.14%**
+* The performance of the filters is deteriorates with the increase of the IN values, however even in this case the
+ vector filter is much better than the row filter.
+
+In the next test we use a complex filter with both AND, and OR to understand the impact of Conjunctive Normal Form on
Review comment:
* evaluation happens on every VectorBatch (1024 rows by default)
##########
File path: java/core/src/java/org/apache/orc/impl/RecordReaderImpl.java
##########
@@ -83,17 +85,21 @@
private final boolean[] fileIncluded;
private final long rowIndexStride;
private long rowInStripe = 0;
+ private long followRowInStripe = 0;
private int currentStripe = -1;
private long rowBaseInStripe = 0;
private long rowCountInStripe = 0;
private final BatchReader reader;
private final OrcIndex indexes;
+ private final boolean[] rowIndexCols;
private final SargApplier sargApp;
// an array about which row groups aren't skipped
private boolean[] includedRowGroups = null;
private final DataReader dataReader;
private final int maxDiskRangeChunkLimit;
private final StripePlanner planner;
+ private final ReadLevel readLevel;
+ private boolean needFollowStripe;
Review comment:
have been read or need to be read?
##########
File path: site/develop/design/lazy_filter.md
##########
@@ -0,0 +1,352 @@
+* [Lazy Filter](#LazyFilter)
+ * [Background](#Background)
+ * [Design](#Design)
+ * [SArg to Filter](#SArgtoFilter)
+ * [Read](#Read)
+ * [Configuration](#Configuration)
+ * [Tests](#Tests)
+ * [Appendix](#Appendix)
+ * [Benchmarks](#Benchmarks)
+ * [Row vs Vector](#RowvsVector)
+ * [Filter](#Filter)
+
+# Lazy Filter <a id="LazyFilter"></a>
+
+## Background <a id="Background"></a>
+
+This feature request started as a result of a large search that is performed with the following characteristics:
+
+* The search fields are not part of partition, bucket or sort fields.
+* The table is a very large table.
+* The predicates result in very few rows compared to the scan size.
+* The search columns are a significant subset of selection columns in the query.
+
+Initial analysis showed that we could have a significant benefit by lazily reading the non-search columns only when we
+have a match. We explore the design and some benchmarks in subsequent sections.
+
+## Design <a id="Design"></a>
+
+This builds further on [ORC-577][ORC-577] which currently only restricts deserialization for some selected data types
+but does not improve on IO.
+
+On a high level the design includes the following components:
+
+```text
+┌──────────────┐ ┌────────────────────────┐
+│ │ │ Read │
+│ │ │ │
+│ │ │ ┌────────────┐ │
+│SArg to Filter│─────────▶│ │Read Filter │ │
+│ │ │ │ Columns │ │
+│ │ │ └────────────┘ │
+│ │ │ │ │
+└──────────────┘ │ ▼ │
+ │ ┌────────────┐ │
+ │ │Apply Filter│ │
+ │ └────────────┘ │
+ │ │ │
+ │ ▼ │
+ │ ┌────────────┐ │
+ │ │Read Select │ │
+ │ │ Columns │ │
+ │ └────────────┘ │
+ │ │
+ │ │
+ └────────────────────────┘
+```
+
+* **SArg to Filter**: Converts Search Arguments passed down into filters for efficient application during scans.
+* **Read**: Performs the lazy read using the filters.
+ * **Read Filter Columns**: Read the filter columns from the file.
+ * **Apply Filter**: Apply the filter on the read filter columns.
+ * **Read Select Columns**: If filter selects at least a row then read the remaining columns.
+
+### SArg to Filter <a id="SArgtoFilter"></a>
+
+SArg to Filter converts the passed SArg into a filter. This enables automatic compatibility with both Spark and Hive as
+they already push down Search Arguments down to ORC.
+
+The SArg is automatically converted into a [Vector Filter][vfilter]. Which is applied during the read process. Two
+filter types were evaluated:
+
+* [Row Filter][rfilter] that evaluates each row across all the predicates once.
+* [Vector Filter][vfilter] that evaluates each filter across the entire vector and adjusts the subsequent evaluation.
+
+While a row based filter is easier to code, it is much [slower][rowvvector] to process. We also see a significant
+[performance gain][rowvvector] in the absence of normalization.
+
+The builder for search argument should allow skipping normalization during the [build][build]. This has already been
+proposed as part of [HIVE-24458][HIVE-24458].
+
+### Read <a id="Read"></a>
+
+The read process has the following changes:
+
+```text
+ │
+ │
+ │
+┌────────────────────────▼────────────────────────┐
+│ ┏━━━━━━━━━━━━━━━━┓ │
+│ ┃Plan ++Search++ ┃ │
+│ ┃ Columns ┃ │
+│ ┗━━━━━━━━━━━━━━━━┛ │
+│ Read │Stripe │
+└────────────────────────┼────────────────────────┘
+ │
+ ▼
+
+
+ │
+ │
+┌────────────────────────▼────────────────────────┐
+│ ┏━━━━━━━━━━━━━━━━┓ │
+│ ┃Read ++Search++ ┃ │
+│ ┃ Columns ┃◀─────────┐ │
+│ ┗━━━━━━━━━━━━━━━━┛ │ │
+│ │ Size = 0 │
+│ ▼ │ │
+│ ┏━━━━━━━━━━━━━━━━┓ │ │
+│ ┃ Apply Filter ┃──────────┘ │
+│ ┗━━━━━━━━━━━━━━━━┛ │
+│ Size > 0 │
+│ │ │
+│ ▼ │
+│ ┏━━━━━━━━━━━━━━━━┓ │
+│ ┃ Plan Select ┃ │
+│ ┃ Columns ┃ │
+│ ┗━━━━━━━━━━━━━━━━┛ │
+│ │ │
+│ ▼ │
+│ ┏━━━━━━━━━━━━━━━━┓ │
+│ ┃ Read Select ┃ │
+│ ┃ Columns ┃ │
+│ ┗━━━━━━━━━━━━━━━━┛ │
+│ Next │Batch │
+└────────────────────────┼────────────────────────┘
+ │
+ ▼
+```
+
+The read process changes:
+
+* **Read Stripe** used to plan the read of all (search + select) columns. This is enhanced to plan and fetch only the
+ search columns. The rest of the stripe planning process optimizations remain unchanged e.g. partial read planning of
+ the stripe based on RowGroup statistics.
+* **Next Batch** identifies the processing that takes place when `RecordReader.nextBatch` is invoked.
+ * **Read Search Columns** takes place instead of reading all the selected columns. This is in sync with the planning
+ that has taken place during **Read Stripe** where only the search columns have been planned.
+ * **Apply Filter** on the batch that at this point only includes search columns. Evaluate the result of the filter:
+ * **Size = 0** indicates all records have been filtered out. Given this we proceed to the next batch of search
+ columns.
+ * **Size > 0** indicates that at least one record accepted by the filter. This record needs to be substantiated with
+ other columns.
+ * **Plan Select Columns** is invoked to perform read of the select columns. The planning happens as follows:
+ * Determine the current position of the read within the stripe and plan the read for the select columns from this
+ point forward to the end of the stripe.
+ * The Read planning of select columns respects the row groups filtered out as a result of the stripe planning.
+ * Fetch the select columns using the above plan.
+ * **Read Select Columns** into the vectorized row batch
+ * Return this batch.
+
+The current implementation performs a single read for the select columns in a stripe.
+
+```text
+┌──────────────────────────────────────────────────┐
+│ ┌────┐ ┌────┐ ┌────┐ ┌────┐ ┌────┐ ┌────┐ ┌────┐ │
+│ │RG0 │ │RG1 │ │RG2■│ │RG3 │ │RG4 │ │RG5■│ │RG6 │ │
+│ └────┘ └────┘ └────┘ └────┘ └────┘ └────┘ └────┘ │
+│ Stripe │
+└──────────────────────────────────────────────────┘
+```
+
+The above diagram depicts a stripe with 7 Row Groups out of which **RG2** and **RG5** are selected by the filter. The
+current implementation does the following:
+
+* Start the read planning process from the first match RG2
+* Read to the end of the stripe that includes RG6
+* Based on the above fetch skips RG0 and RG1 subject to compression block boundaries
+
+The above logic could be enhanced to perform say **2 or n** reads before reading to the end of stripe. The current
+implementation allows 0 reads before reading to the end of the stripe. The value of **n** could be configurable but
+should avoid too many short reads.
+
+The read behavior changes as follows with multiple reads being allowed within a stripe for select columns:
+
+```text
+┌──────────────────────────────────────────────────┐
+│ ┌────┐ ┌────┐ ┌────┐ ┌────┐ ┌────┐ ┌────┐ ┌────┐ │
+│ │ │ │ │ │■■■■│ │■■■■│ │■■■■│ │■■■■│ │■■■■│ │
+│ └────┘ └────┘ └────┘ └────┘ └────┘ └────┘ └────┘ │
+│ Current implementation │
+└──────────────────────────────────────────────────┘
+┌──────────────────────────────────────────────────┐
+│ ┌────┐ ┌────┐ ┌────┐ ┌────┐ ┌────┐ ┌────┐ ┌────┐ │
+│ │ │ │ │ │■■■■│ │ │ │ │ │■■■■│ │■■■■│ │
+│ └────┘ └────┘ └────┘ └────┘ └────┘ └────┘ └────┘ │
+│ Allow 1 partial read │
+└──────────────────────────────────────────────────┘
+```
+
+The figure shows that we could read significantly fewer bytes by performing an additional read before reading to the end
Review comment:
Is partial read conf part of ORC-743?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
pavibhai commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r574124194
##########
File path: java/core/src/java/org/apache/orc/impl/BufferChunk.java
##########
@@ -69,22 +69,8 @@ public final String toString() {
@Override
public DiskRange sliceAndShift(long offset, long end, long shiftBy) {
- assert offset <= end && offset >= this.offset && end <= this.end;
- assert offset + shiftBy >= 0;
- ByteBuffer sliceBuf = chunk.slice();
- int newPos = (int) (offset - this.offset);
- int newLimit = newPos + (int) (end - offset);
- try {
- sliceBuf.position(newPos);
- sliceBuf.limit(newLimit);
- } catch (Throwable t) {
- LOG.error("Failed to slice buffer chunk with range" + " [" + this.offset + ", " + this.end
- + "), position: " + chunk.position() + " limit: " + chunk.limit() + ", "
- + (chunk.isDirect() ? "direct" : "array") + "; to [" + offset + ", " + end + ") "
- + t.getClass());
- throw new RuntimeException(t);
- }
- return new BufferChunk(sliceBuf, offset + shiftBy);
+ // Method has no code references
+ throw new UnsupportedOperationException();
Review comment:
I have gone through the change again carefully. This is not required for the current implementation, so I will revert this change here and we can take this up independently if needed
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai edited a comment on pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
pavibhai edited a comment on pull request #635:
URL: https://github.com/apache/orc/pull/635#issuecomment-768514501
> Could your resolve conflicts by rebasing to the master branch?
sure, done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] dongjoon-hyun commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r566565971
##########
File path: java/core/src/java/org/apache/orc/impl/ReaderImpl.java
##########
@@ -74,7 +74,7 @@
protected List<OrcProto.StripeStatistics> stripeStatistics;
private final int metadataSize;
protected final List<OrcProto.Type> types;
- private TypeDescription schema;
+ private final TypeDescription schema;
Review comment:
Let's revert this change.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
pavibhai commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r578838632
##########
File path: java/core/src/java/org/apache/orc/impl/RecordReaderImpl.java
##########
@@ -83,17 +85,21 @@
private final boolean[] fileIncluded;
private final long rowIndexStride;
private long rowInStripe = 0;
+ private long followRowInStripe = 0;
private int currentStripe = -1;
private long rowBaseInStripe = 0;
private long rowCountInStripe = 0;
private final BatchReader reader;
private final OrcIndex indexes;
+ private final boolean[] rowIndexCols;
private final SargApplier sargApp;
// an array about which row groups aren't skipped
private boolean[] includedRowGroups = null;
private final DataReader dataReader;
private final int maxDiskRangeChunkLimit;
private final StripePlanner planner;
+ private final ReadLevel readLevel;
+ private boolean needFollowStripe;
Review comment:
renamed to needsFollowColumnsRead
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
pavibhai commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r589648917
##########
File path: java/core/src/java/org/apache/orc/impl/reader/StripePlanner.java
##########
@@ -68,14 +74,15 @@
private String writerTimezone;
private long currentStripeId;
private long originalStripeId;
- private Map<StreamName, StreamInformation> streams = new HashMap<>();
+ private final Map<StreamName, StreamInformation> streams = new HashMap<>();
Review comment:
Merged as part of ORC-754
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] dongjoon-hyun commented on pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on pull request #635:
URL: https://github.com/apache/orc/pull/635#issuecomment-778528402
@pavibhai . FYI, Owen seems to change `master` massively soon in the following PR.
- https://github.com/apache/orc/pull/641
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
pavibhai commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r589642006
##########
File path: java/core/src/java/org/apache/orc/impl/OrcAcidUtils.java
##########
@@ -68,7 +69,7 @@ public static long getLastFlushLength(FileSystem fs,
}
}
- private static final Charset utf8 = Charset.forName("UTF-8");
+ private static final Charset utf8 = StandardCharsets.UTF_8;
Review comment:
Merged as part of ORC-754
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
pavibhai commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r567132476
##########
File path: java/core/src/java/org/apache/orc/impl/TreeReaderFactory.java
##########
@@ -67,7 +70,7 @@
Set<Integer> getColumnFilterIds();
- Consumer<VectorizedRowBatch> getColumnFilterCallback();
+ Consumer<org.apache.orc.filter.FilterContext> getColumnFilterCallback();
Review comment:
Will change to OrcFilterContext
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] dongjoon-hyun commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r566566087
##########
File path: java/core/src/java/org/apache/orc/impl/RecordReaderUtils.java
##########
@@ -545,7 +545,7 @@ public int hashCode() {
private long currentGeneration = 0;
- private final TreeMap<Key, ByteBuffer> getBufferTree(boolean direct) {
Review comment:
Let's revert this change.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
pavibhai commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r574099058
##########
File path: java/core/src/java/org/apache/orc/impl/RecordReaderUtils.java
##########
@@ -545,7 +545,7 @@ public int hashCode() {
private long currentGeneration = 0;
- private final TreeMap<Key, ByteBuffer> getBufferTree(boolean direct) {
Review comment:
Any reason for revert or you prefer this also goes into a separate PR?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
pavibhai commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r600002490
##########
File path: java/core/src/java/org/apache/orc/impl/reader/tree/LevelTypeReader.java
##########
@@ -0,0 +1,107 @@
+/*
+ * 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.orc.impl.reader.tree;
+
+import org.apache.hadoop.hive.ql.exec.vector.ColumnVector;
+import org.apache.hadoop.hive.ql.io.filter.FilterContext;
+import org.apache.orc.OrcProto;
+import org.apache.orc.impl.PositionProvider;
+import org.apache.orc.impl.reader.StripePlanner;
+
+import java.io.IOException;
+
+/**
+ * Wrapper reader that implements the level logic.
+ * The methods are invoked on the reader only if the read level matches.
+ */
+public class LevelTypeReader implements TypeReader {
+ private final TypeReader reader;
Review comment:
LevelTypeReader has been removed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai commented on pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
pavibhai commented on pull request #635:
URL: https://github.com/apache/orc/pull/635#issuecomment-781490401
> Left some initial comments here and there but in general I will have to agree with the existing comments that this is just to hard to review this PR and needs splitting. I would propose the following split:
Thanks for your feedback and suggestions @pgaref
>
I would like to propose updates to checkstyle based on line and indentation feedback I have received on this PR.
> * Findbugs onliner
Can you please clarify what you mean by this?
>
> * Changes to MapReduce classes
>
> * Introduce OrcFilterContext (also affects row-filter tests etc. but is manageable)
>
> * Introduce ReadLevel on Reader and Planner
>
> * Introduce [ORC-743](https://issues.apache.org/jira/browse/ORC-743) changes to the planner (partial-reads?)
[ORC-743](https://issues.apache.org/jira/browse/ORC-743) includes SArg to Filter conversion, the partial reads are part of this PR. The configuration for partial reads is absent in both the PRs right now.
I will treat this as introduce partial reads, followed by [ORC-743](https://issues.apache.org/jira/browse/ORC-743) which might have other splits that it needs.
>
> * Documentation changes after we merge all the expected functionality
>
> * Code changes like making a variable final or using the UTF8 Character Set, or new methods (less critical)
Any changes that are not required we can pull it out to an unrelated PR. I would like to include methods that avoid code duplication into the PR still.
>
>
> Please let me know what you think and again thanks for all the work!
Hope that makes sense. Please let me know.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
pavibhai commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r578853891
##########
File path: java/core/src/java/org/apache/orc/impl/TreeReaderFactory.java
##########
@@ -2854,11 +2924,19 @@ public static TypeReader createTreeReader(TypeDescription readerType,
}
}
+ public static boolean isDecimalAsLong(OrcFile.Version version, int precision) {
+ return version == OrcFile.Version.UNSTABLE_PRE_2_0 &&
+ precision <= TypeDescription.MAX_DECIMAL64_PRECISION;
+ }
+
public static BatchReader createRootReader(TypeDescription readerType, Context context)
throws IOException {
TypeReader reader = createTreeReader(readerType, context);
if (reader instanceof StructTreeReader) {
- return new StructBatchReader((StructTreeReader) reader, context);
+ return new StructBatchReader(reader, context);
+ } else if (reader instanceof LevelTypeReader
+ && ((LevelTypeReader)reader).getReader() instanceof StructTreeReader) {
+ return new LevelStructBatchReader((LevelTypeReader) reader, context);
Review comment:
I understand the concern regarding doubling the type readers. I don't quite understand the suggestion on how to circumvent that.
One possibility I do see is that instead of having the conditional code within LevelTypeReader, I can add new final methods in the TreeReader base class that perform this conditional logic and this method then invokes the corresponding methods
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] dongjoon-hyun commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r565703336
##########
File path: java/core/src/java/org/apache/orc/Reader.java
##########
@@ -265,7 +269,7 @@ public Options schema(TypeDescription schema) {
*
* @return this
*/
- public Options setRowFilter(String[] filterColumnNames, Consumer<VectorizedRowBatch> filterCallback) {
+ public Options setRowFilter(String[] filterColumnNames, Consumer<FilterContext> filterCallback) {
Review comment:
This looks like a breaking API change.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
pavibhai commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r574100024
##########
File path: java/core/src/java/org/apache/orc/impl/reader/tree/TypeReader.java
##########
@@ -28,18 +28,20 @@
public interface TypeReader {
void checkEncoding(OrcProto.ColumnEncoding encoding) throws IOException;
- void startStripe(StripePlanner planner) throws IOException;
+ void startStripe(StripePlanner planner, ReadLevel readLevel) throws IOException;
- void seek(PositionProvider[] index) throws IOException;
+ void seek(PositionProvider[] index, ReadLevel readLevel) throws IOException;
- void seek(PositionProvider index) throws IOException;
+ void seek(PositionProvider index, ReadLevel readLevel) throws IOException;
- void skipRows(long rows) throws IOException;
+ void skipRows(long rows, ReadLevel readLevel) throws IOException;
void nextVector(ColumnVector previous,
boolean[] isNull,
int batchSize,
- FilterContext filterContext) throws IOException;
+ FilterContext filterContext, ReadLevel readLevel) throws IOException;
Review comment:
done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] dongjoon-hyun commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r566565876
##########
File path: java/core/src/java/org/apache/orc/impl/OrcAcidUtils.java
##########
@@ -68,7 +69,7 @@ public static long getLastFlushLength(FileSystem fs,
}
}
- private static final Charset utf8 = Charset.forName("UTF-8");
+ private static final Charset utf8 = StandardCharsets.UTF_8;
Review comment:
Please make another PR for this file change. We can merge that first. There is only one instance, right?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
pavibhai commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r577088265
##########
File path: java/core/src/java/org/apache/orc/filter/OrcFilterContext.java
##########
@@ -0,0 +1,169 @@
+/*
+ * 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.orc.filter;
+
+import org.apache.hadoop.hive.ql.exec.vector.ColumnVector;
+import org.apache.hadoop.hive.ql.exec.vector.StructColumnVector;
+import org.apache.hadoop.hive.ql.exec.vector.VectorizedRowBatch;
+import org.apache.hadoop.hive.ql.io.filter.MutableFilterContext;
+import org.apache.orc.TypeDescription;
+import org.jetbrains.annotations.NotNull;
+
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.regex.Pattern;
+
+/**
+ * This defines the input for any filter operation.
+ * <p>
+ * It offers a convenience method for finding the column vector from a given name, that the filters
+ * can invoke to get access to the column vector.
+ */
+public class OrcFilterContext implements MutableFilterContext {
+
+ VectorizedRowBatch batch = null;
+ // Cache of field to ColumnVector, this is reset everytime the batch reference changes
+ private final Map<String, ColumnVector> vectors;
+ private final TypeDescription readSchema;
+
+ public OrcFilterContext(TypeDescription readSchema) {
+ this.readSchema = readSchema;
+ this.vectors = new HashMap<>();
+ }
+
+ public OrcFilterContext setBatch(@NotNull VectorizedRowBatch batch) {
+ if (batch != this.batch) {
+ this.batch = batch;
+ vectors.clear();
+ }
+ return this;
+ }
+
+ @Override
+ public void setFilterContext(boolean selectedInUse, int[] selected, int selectedSize) {
+ batch.setFilterContext(selectedInUse, selected, selectedSize);
+ }
+
+ @Override
+ public boolean validateSelected() {
+ return batch.validateSelected();
+ }
+
+ @Override
+ public int[] updateSelected(int i) {
+ return batch.updateSelected(i);
+ }
+
+ @Override
+ public void setSelectedInUse(boolean b) {
+ batch.setSelectedInUse(b);
+ }
+
+ @Override
+ public void setSelected(int[] ints) {
+ batch.setSelected(ints);
+ }
+
+ @Override
+ public void setSelectedSize(int i) {
+ batch.setSelectedSize(i);
+ }
+
+ @Override
+ public void reset() {
+ batch.reset();
+ }
+
+ @Override
+ public boolean isSelectedInUse() {
+ return batch.isSelectedInUse();
+ }
+
+ @Override
+ public int[] getSelected() {
+ return batch.getSelected();
+ }
+
+ @Override
+ public int getSelectedSize() {
+ return batch.getSelectedSize();
+ }
+
+ public ColumnVector[] getCols() {
+ return batch.cols;
+ }
+
+ /**
+ * Retrieves the column vector that matches the specified name. Allows support for nested struct
+ * references e.g. order.date where data is a field in a struct called order.
+ *
+ * @param name The column name whose vector should be retrieved
+ * @return The column vector
+ * @throws IllegalArgumentException if the field is not found or if the nested field is not part
+ * of a struct
+ */
+ public ColumnVector findColumnVector(String name) {
+ if (!vectors.containsKey(name)) {
+ vectors.put(name, findVector(name));
+ }
+
+ return vectors.get(name);
+ }
+
+ private ColumnVector findVector(String name) {
+ String[] refs = name.split(SPLIT_ON_PERIOD);
+ TypeDescription schema = readSchema;
+ List<String> fNames = schema.getFieldNames();
+ ColumnVector[] cols = batch.cols;
+ ColumnVector vector;
+
+ // Try to find the required column at the top level
+ int idx = findVectorIdx(refs[0], fNames);
+ vector = cols[idx];
+
+ // In case we are asking for a nested search further. Only nested struct references are allowed.
+ for (int i = 1; i < refs.length; i++) {
+ schema = schema.getChildren().get(idx);
+ if (schema.getCategory() != TypeDescription.Category.STRUCT) {
+ throw new IllegalArgumentException(String.format(
+ "Field %s:%s not a struct field does not allow nested reference",
+ refs[i - 1],
+ schema));
+ }
+ cols = ((StructColumnVector) vector).fields;
+ vector = cols[findVectorIdx(refs[i], schema.getFieldNames())];
+ }
+
+ return vector;
+ }
+
+ private static int findVectorIdx(String name, List<String> fieldNames) {
+ int idx = fieldNames.indexOf(name);
+ if (idx < 0) {
+ throw new IllegalArgumentException(String.format("Field %s not found in %s",
Review comment:
Here the focus is on the read schema. Schema evolution will already map the file schema into the read schema.
So in case the read schema has the column even if the file does not have the column the vector will be found and returned.
The case that is not supported is about find a column absent in the read schema.
##########
File path: java/core/src/java/org/apache/orc/filter/OrcFilterContext.java
##########
@@ -0,0 +1,169 @@
+/*
+ * 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.orc.filter;
+
+import org.apache.hadoop.hive.ql.exec.vector.ColumnVector;
+import org.apache.hadoop.hive.ql.exec.vector.StructColumnVector;
+import org.apache.hadoop.hive.ql.exec.vector.VectorizedRowBatch;
+import org.apache.hadoop.hive.ql.io.filter.MutableFilterContext;
+import org.apache.orc.TypeDescription;
+import org.jetbrains.annotations.NotNull;
+
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.regex.Pattern;
+
+/**
+ * This defines the input for any filter operation.
+ * <p>
+ * It offers a convenience method for finding the column vector from a given name, that the filters
+ * can invoke to get access to the column vector.
+ */
+public class OrcFilterContext implements MutableFilterContext {
+
+ VectorizedRowBatch batch = null;
+ // Cache of field to ColumnVector, this is reset everytime the batch reference changes
+ private final Map<String, ColumnVector> vectors;
+ private final TypeDescription readSchema;
+
+ public OrcFilterContext(TypeDescription readSchema) {
+ this.readSchema = readSchema;
+ this.vectors = new HashMap<>();
+ }
+
+ public OrcFilterContext setBatch(@NotNull VectorizedRowBatch batch) {
+ if (batch != this.batch) {
+ this.batch = batch;
+ vectors.clear();
+ }
+ return this;
+ }
+
+ @Override
+ public void setFilterContext(boolean selectedInUse, int[] selected, int selectedSize) {
+ batch.setFilterContext(selectedInUse, selected, selectedSize);
+ }
+
+ @Override
+ public boolean validateSelected() {
+ return batch.validateSelected();
+ }
+
+ @Override
+ public int[] updateSelected(int i) {
+ return batch.updateSelected(i);
+ }
+
+ @Override
+ public void setSelectedInUse(boolean b) {
+ batch.setSelectedInUse(b);
+ }
+
+ @Override
+ public void setSelected(int[] ints) {
+ batch.setSelected(ints);
+ }
+
+ @Override
+ public void setSelectedSize(int i) {
+ batch.setSelectedSize(i);
+ }
+
+ @Override
+ public void reset() {
+ batch.reset();
+ }
+
+ @Override
+ public boolean isSelectedInUse() {
+ return batch.isSelectedInUse();
+ }
+
+ @Override
+ public int[] getSelected() {
+ return batch.getSelected();
+ }
+
+ @Override
+ public int getSelectedSize() {
+ return batch.getSelectedSize();
+ }
+
+ public ColumnVector[] getCols() {
+ return batch.cols;
+ }
+
+ /**
+ * Retrieves the column vector that matches the specified name. Allows support for nested struct
+ * references e.g. order.date where data is a field in a struct called order.
+ *
+ * @param name The column name whose vector should be retrieved
+ * @return The column vector
+ * @throws IllegalArgumentException if the field is not found or if the nested field is not part
+ * of a struct
+ */
+ public ColumnVector findColumnVector(String name) {
+ if (!vectors.containsKey(name)) {
+ vectors.put(name, findVector(name));
+ }
+
+ return vectors.get(name);
+ }
+
+ private ColumnVector findVector(String name) {
+ String[] refs = name.split(SPLIT_ON_PERIOD);
+ TypeDescription schema = readSchema;
+ List<String> fNames = schema.getFieldNames();
+ ColumnVector[] cols = batch.cols;
+ ColumnVector vector;
+
+ // Try to find the required column at the top level
+ int idx = findVectorIdx(refs[0], fNames);
+ vector = cols[idx];
+
+ // In case we are asking for a nested search further. Only nested struct references are allowed.
+ for (int i = 1; i < refs.length; i++) {
+ schema = schema.getChildren().get(idx);
+ if (schema.getCategory() != TypeDescription.Category.STRUCT) {
+ throw new IllegalArgumentException(String.format(
+ "Field %s:%s not a struct field does not allow nested reference",
+ refs[i - 1],
+ schema));
+ }
+ cols = ((StructColumnVector) vector).fields;
+ vector = cols[findVectorIdx(refs[i], schema.getFieldNames())];
+ }
+
+ return vector;
+ }
+
+ private static int findVectorIdx(String name, List<String> fieldNames) {
+ int idx = fieldNames.indexOf(name);
+ if (idx < 0) {
+ throw new IllegalArgumentException(String.format("Field %s not found in %s",
Review comment:
Here the focus is on the read schema. Schema evolution will already map the file schema into the read schema.
So in case the read schema has the column even if the file does not have the column the vector will be found and returned.
The case that is not supported is about finding a column absent in the read schema.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] dongjoon-hyun commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r566566954
##########
File path: java/core/src/java/org/apache/orc/impl/reader/tree/ReadLevel.java
##########
@@ -0,0 +1,25 @@
+/*
+ * 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.orc.impl.reader.tree;
+
+public enum ReadLevel {
+ ALL, // Read every column
+ LEAD, // Read only the filter columns
+ FOLLOW // Read the non-filter columns
Review comment:
Comment indentation is not aligned.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
pavibhai commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r577089054
##########
File path: java/core/src/java/org/apache/orc/impl/RecordReaderImpl.java
##########
@@ -83,17 +85,21 @@
private final boolean[] fileIncluded;
private final long rowIndexStride;
private long rowInStripe = 0;
+ private long followRowInStripe = 0;
private int currentStripe = -1;
private long rowBaseInStripe = 0;
private long rowCountInStripe = 0;
private final BatchReader reader;
private final OrcIndex indexes;
+ private final boolean[] rowIndexCols;
Review comment:
will do
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
pavibhai commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r589649352
##########
File path: java/core/src/java/org/apache/orc/filter/OrcFilterContext.java
##########
@@ -0,0 +1,169 @@
+/*
+ * 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.orc.filter;
+
+import org.apache.hadoop.hive.ql.exec.vector.ColumnVector;
+import org.apache.hadoop.hive.ql.exec.vector.StructColumnVector;
+import org.apache.hadoop.hive.ql.exec.vector.VectorizedRowBatch;
+import org.apache.hadoop.hive.ql.io.filter.MutableFilterContext;
+import org.apache.orc.TypeDescription;
+import org.jetbrains.annotations.NotNull;
+
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.regex.Pattern;
+
+/**
+ * This defines the input for any filter operation.
+ * <p>
+ * It offers a convenience method for finding the column vector from a given name, that the filters
+ * can invoke to get access to the column vector.
+ */
+public class OrcFilterContext implements MutableFilterContext {
Review comment:
Merged as part of ORC-755
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
pavibhai commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r589642177
##########
File path: java/core/src/java/org/apache/orc/impl/ReaderImpl.java
##########
@@ -74,7 +74,7 @@
protected List<OrcProto.StripeStatistics> stripeStatistics;
private final int metadataSize;
protected final List<OrcProto.Type> types;
- private TypeDescription schema;
+ private final TypeDescription schema;
Review comment:
Merged as part of ORC-754
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
pavibhai commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r600001439
##########
File path: java/core/src/java/org/apache/orc/impl/reader/tree/ReadLevel.java
##########
@@ -0,0 +1,25 @@
+/*
+ * 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.orc.impl.reader.tree;
+
+public enum ReadLevel {
Review comment:
Pushing this into TypeReader as LevelTypeReader is no longer present.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
pavibhai commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r578825345
##########
File path: java/core/src/java/org/apache/orc/impl/RecordReaderImpl.java
##########
@@ -83,17 +85,24 @@
private final boolean[] fileIncluded;
private final long rowIndexStride;
private long rowInStripe = 0;
+ private long followRowInStripe = 0;
private int currentStripe = -1;
private long rowBaseInStripe = 0;
private long rowCountInStripe = 0;
private final BatchReader reader;
private final OrcIndex indexes;
+ // identifies the columns requiring row indexes
+ private final boolean[] rowIndexCols;
private final SargApplier sargApp;
// an array about which row groups aren't skipped
private boolean[] includedRowGroups = null;
private final DataReader dataReader;
private final int maxDiskRangeChunkLimit;
private final StripePlanner planner;
+ // identifies the type of read: FULL (no filters), LEAD (filters are present)
+ private final ReadLevel readLevel;
Review comment:
FOLLOW is never set at the RecordReader level. It will only be FULL or LEAD. If this is LEAD then as part of read FOLLOW is implicitly used.
I can clarify further in the comment if that helps.
##########
File path: java/core/src/java/org/apache/orc/impl/BufferChunkList.java
##########
@@ -22,6 +22,14 @@
* Builds a list of buffer chunks
*/
public class BufferChunkList {
+ public BufferChunk getHead() {
Review comment:
Removed
##########
File path: java/core/src/java/org/apache/orc/impl/reader/StripePlanner.java
##########
@@ -363,11 +396,26 @@ private void addChunk(BufferChunkList list, StreamInformation stream,
stream.firstChunk = chunk;
}
list.add(chunk);
+ stream.lastChunk = chunk;
offset += thisLen;
length -= thisLen;
}
}
+ private BufferChunkList addFollowChunk(long offset, long length) {
Review comment:
Removed
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
pavibhai commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r577089119
##########
File path: java/core/src/java/org/apache/orc/impl/RecordReaderImpl.java
##########
@@ -83,17 +85,21 @@
private final boolean[] fileIncluded;
private final long rowIndexStride;
private long rowInStripe = 0;
+ private long followRowInStripe = 0;
private int currentStripe = -1;
private long rowBaseInStripe = 0;
private long rowCountInStripe = 0;
private final BatchReader reader;
private final OrcIndex indexes;
+ private final boolean[] rowIndexCols;
private final SargApplier sargApp;
// an array about which row groups aren't skipped
private boolean[] includedRowGroups = null;
private final DataReader dataReader;
private final int maxDiskRangeChunkLimit;
private final StripePlanner planner;
+ private final ReadLevel readLevel;
+ private boolean needFollowStripe;
Review comment:
will do
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] dongjoon-hyun commented on pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on pull request #635:
URL: https://github.com/apache/orc/pull/635#issuecomment-768458891
Could your resolve conflicts by rebasing to the master branch?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
pavibhai commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r600000623
##########
File path: java/core/src/java/org/apache/orc/impl/RecordReaderImpl.java
##########
@@ -234,11 +246,17 @@ protected RecordReaderImpl(ReaderImpl fileReader,
// If the column is not present in the file then this can be ignored from read.
if (expandColId != -1) {
filterColIds.add(expandColId);
+ rowIndexCols[expandColId] = true;
}
}
LOG.info("Filter Columns: " + filterColIds);
+ this.readLevel = ReadLevel.LEAD;
+ } else {
+ this.readLevel = ReadLevel.ALL;
Review comment:
ReadLevel is now an EnumSet
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
pavibhai commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r578764623
##########
File path: java/core/src/test/org/apache/orc/TestRowFilteringIOSkip.java
##########
@@ -0,0 +1,415 @@
+/*
+ * 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.orc;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.FileSystem;
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.hive.ql.exec.vector.BytesColumnVector;
+import org.apache.hadoop.hive.ql.exec.vector.DecimalColumnVector;
+import org.apache.hadoop.hive.ql.exec.vector.LongColumnVector;
+import org.apache.hadoop.hive.ql.exec.vector.VectorizedRowBatch;
+import org.apache.hadoop.hive.ql.io.sarg.PredicateLeaf;
+import org.apache.hadoop.hive.ql.io.sarg.SearchArgument;
+import org.apache.hadoop.hive.ql.io.sarg.SearchArgumentFactory;
+import org.apache.hadoop.hive.serde2.io.HiveDecimalWritable;
+import org.apache.orc.filter.OrcFilterContext;
+import org.junit.Assert;
+import org.junit.BeforeClass;
+import org.junit.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.util.HashSet;
+import java.util.Random;
+import java.util.Set;
+import java.util.function.Consumer;
+
+public class TestRowFilteringIOSkip {
+ private final static Logger LOG = LoggerFactory.getLogger(TestRowFilteringIOSkip.class);
+ private static final Path workDir = new Path(System.getProperty("test.tmp.dir",
+ "target" + File.separator + "test"
Review comment:
I was aligning the parameters with indentation.
Please let me know what is expected in terms of indentation?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
pavibhai commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r589648572
##########
File path: java/core/src/java/org/apache/orc/impl/TreeReaderFactory.java
##########
@@ -245,7 +263,8 @@ protected static IntegerReader createIntegerReader(OrcProto.ColumnEncoding.Kind
switch (kind) {
case DIRECT_V2:
case DICTIONARY_V2:
- return new RunLengthIntegerReaderV2(in, signed, context == null ? false : context.isSkipCorrupt());
+ return new RunLengthIntegerReaderV2(in, signed,
Review comment:
Merged as part of ORC-754
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
pavibhai commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r589642384
##########
File path: java/core/src/java/org/apache/orc/impl/RecordReaderUtils.java
##########
@@ -545,7 +545,7 @@ public int hashCode() {
private long currentGeneration = 0;
- private final TreeMap<Key, ByteBuffer> getBufferTree(boolean direct) {
Review comment:
Merged as part of ORC-754
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
pavibhai commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r574098545
##########
File path: java/core/src/java/org/apache/orc/filter/FilterContext.java
##########
@@ -0,0 +1,169 @@
+/*
+ * 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.orc.filter;
+
+import org.apache.hadoop.hive.ql.exec.vector.ColumnVector;
+import org.apache.hadoop.hive.ql.exec.vector.StructColumnVector;
+import org.apache.hadoop.hive.ql.exec.vector.VectorizedRowBatch;
+import org.apache.hadoop.hive.ql.io.filter.MutableFilterContext;
+import org.apache.orc.TypeDescription;
+import org.jetbrains.annotations.NotNull;
+
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.regex.Pattern;
+
+/**
+ * This defines the input for any filter operation.
+ *
+ * It offers a convenience method for finding the column vector from a given name, that the filters
+ * can invoke to get access to the column vector.
+ */
+public class FilterContext implements MutableFilterContext {
Review comment:
I think this is better suited in the ORC project as it is explicitly about ORC Filter handling.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
pavibhai commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r589641723
##########
File path: java/core/src/java/org/apache/orc/filter/FilterContext.java
##########
@@ -0,0 +1,169 @@
+/*
+ * 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.orc.filter;
+
+import org.apache.hadoop.hive.ql.exec.vector.ColumnVector;
+import org.apache.hadoop.hive.ql.exec.vector.StructColumnVector;
+import org.apache.hadoop.hive.ql.exec.vector.VectorizedRowBatch;
+import org.apache.hadoop.hive.ql.io.filter.MutableFilterContext;
+import org.apache.orc.TypeDescription;
+import org.jetbrains.annotations.NotNull;
+
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.regex.Pattern;
+
+/**
+ * This defines the input for any filter operation.
+ *
+ * It offers a convenience method for finding the column vector from a given name, that the filters
+ * can invoke to get access to the column vector.
+ */
+public class FilterContext implements MutableFilterContext {
Review comment:
Merged as part of ORC-755
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
pavibhai commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r574097768
##########
File path: java/core/src/java/org/apache/orc/impl/reader/StripePlanner.java
##########
@@ -144,10 +165,23 @@ public StripePlanner parseStripe(StripeInformation stripe,
* @return the buffers that were read
*/
public BufferChunkList readData(OrcIndex index,
- boolean[] rowGroupInclude,
- boolean forceDirect) throws IOException {
+ boolean[] rowGroupInclude,
+ boolean forceDirect) throws IOException {
+ ReadLevel readLevel = filterColIds.isEmpty() ? ReadLevel.ALL : ReadLevel.LEAD;
BufferChunkList chunks = (index == null || rowGroupInclude == null)
- ? planDataReading() : planPartialDataReading(index, rowGroupInclude);
+ ? planDataReading(readLevel) : planPartialDataReading(index, rowGroupInclude, readLevel);
+ dataReader.readFileData(chunks, forceDirect);
+ return chunks;
+ }
+
+ public BufferChunkList readFollowData(OrcIndex index,
+ boolean[] rowGroupInclude,
Review comment:
done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
pavibhai commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r566065779
##########
File path: java/core/src/java/org/apache/orc/Reader.java
##########
@@ -265,7 +269,7 @@ public Options schema(TypeDescription schema) {
*
* @return this
*/
- public Options setRowFilter(String[] filterColumnNames, Consumer<VectorizedRowBatch> filterCallback) {
+ public Options setRowFilter(String[] filterColumnNames, Consumer<FilterContext> filterCallback) {
Review comment:
Yes this is a breaking change. I thought the convenience of retrieving the column vectors via name does ease the complexity of filter development.
I am open to other ideas on achieving the same.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
pavibhai commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r578766105
##########
File path: java/core/src/findbugs/exclude.xml
##########
@@ -70,4 +70,8 @@
<Class name="org.apache.orc.TypeDescription"/>
<Method name="equals" />
</Match>
+ <Match>
+ <Bug pattern="URF_UNREAD_PUBLIC_OR_PROTECTED_FIELD"/>
+ <Class name="org.apache.orc.impl.reader.StripePlanner$StreamInformation"/>
+ </Match>
Review comment:
Removing this, not needed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] wgtmac commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
wgtmac commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r576843979
##########
File path: site/develop/design/lazy_filter.md
##########
@@ -0,0 +1,352 @@
+* [Lazy Filter](#LazyFilter)
+ * [Background](#Background)
+ * [Design](#Design)
+ * [SArg to Filter](#SArgtoFilter)
+ * [Read](#Read)
+ * [Configuration](#Configuration)
+ * [Tests](#Tests)
+ * [Appendix](#Appendix)
+ * [Benchmarks](#Benchmarks)
+ * [Row vs Vector](#RowvsVector)
+ * [Filter](#Filter)
+
+# Lazy Filter <a id="LazyFilter"></a>
+
+## Background <a id="Background"></a>
+
+This feature request started as a result of a large search that is performed with the following characteristics:
+
+* The search fields are not part of partition, bucket or sort fields.
+* The table is a very large table.
+* The predicates result in very few rows compared to the scan size.
+* The search columns are a significant subset of selection columns in the query.
+
+Initial analysis showed that we could have a significant benefit by lazily reading the non-search columns only when we
+have a match. We explore the design and some benchmarks in subsequent sections.
+
+## Design <a id="Design"></a>
+
+This builds further on [ORC-577][ORC-577] which currently only restricts deserialization for some selected data types
+but does not improve on IO.
+
+On a high level the design includes the following components:
+
+```text
+┌──────────────┐ ┌────────────────────────┐
+│ │ │ Read │
+│ │ │ │
+│ │ │ ┌────────────┐ │
+│SArg to Filter│─────────▶│ │Read Filter │ │
+│ │ │ │ Columns │ │
+│ │ │ └────────────┘ │
+│ │ │ │ │
+└──────────────┘ │ ▼ │
+ │ ┌────────────┐ │
+ │ │Apply Filter│ │
+ │ └────────────┘ │
+ │ │ │
+ │ ▼ │
+ │ ┌────────────┐ │
+ │ │Read Select │ │
+ │ │ Columns │ │
+ │ └────────────┘ │
+ │ │
+ │ │
+ └────────────────────────┘
+```
+
+* **SArg to Filter**: Converts Search Arguments passed down into filters for efficient application during scans.
+* **Read**: Performs the lazy read using the filters.
+ * **Read Filter Columns**: Read the filter columns from the file.
+ * **Apply Filter**: Apply the filter on the read filter columns.
+ * **Read Select Columns**: If filter selects at least a row then read the remaining columns.
+
+### SArg to Filter <a id="SArgtoFilter"></a>
+
+SArg to Filter converts the passed SArg into a filter. This enables automatic compatibility with both Spark and Hive as
+they already push down Search Arguments down to ORC.
+
+The SArg is automatically converted into a [Vector Filter][vfilter]. Which is applied during the read process. Two
+filter types were evaluated:
+
+* [Row Filter][rfilter] that evaluates each row across all the predicates once.
+* [Vector Filter][vfilter] that evaluates each filter across the entire vector and adjusts the subsequent evaluation.
+
+While a row based filter is easier to code, it is much [slower][rowvvector] to process. We also see a significant
+[performance gain][rowvvector] in the absence of normalization.
+
+The builder for search argument should allow skipping normalization during the [build][build]. This has already been
+proposed as part of [HIVE-24458][HIVE-24458].
+
+### Read <a id="Read"></a>
+
+The read process has the following changes:
+
+```text
+ │
+ │
+ │
+┌────────────────────────▼────────────────────────┐
+│ ┏━━━━━━━━━━━━━━━━┓ │
+│ ┃Plan ++Search++ ┃ │
+│ ┃ Columns ┃ │
+│ ┗━━━━━━━━━━━━━━━━┛ │
+│ Read │Stripe │
+└────────────────────────┼────────────────────────┘
+ │
+ ▼
+
+
+ │
+ │
+┌────────────────────────▼────────────────────────┐
+│ ┏━━━━━━━━━━━━━━━━┓ │
+│ ┃Read ++Search++ ┃ │
+│ ┃ Columns ┃◀─────────┐ │
+│ ┗━━━━━━━━━━━━━━━━┛ │ │
+│ │ Size = 0 │
+│ ▼ │ │
+│ ┏━━━━━━━━━━━━━━━━┓ │ │
+│ ┃ Apply Filter ┃──────────┘ │
+│ ┗━━━━━━━━━━━━━━━━┛ │
+│ Size > 0 │
+│ │ │
+│ ▼ │
+│ ┏━━━━━━━━━━━━━━━━┓ │
+│ ┃ Plan Select ┃ │
+│ ┃ Columns ┃ │
+│ ┗━━━━━━━━━━━━━━━━┛ │
+│ │ │
+│ ▼ │
+│ ┏━━━━━━━━━━━━━━━━┓ │
+│ ┃ Read Select ┃ │
+│ ┃ Columns ┃ │
+│ ┗━━━━━━━━━━━━━━━━┛ │
+│ Next │Batch │
+└────────────────────────┼────────────────────────┘
+ │
+ ▼
+```
+
+The read process changes:
+
+* **Read Stripe** used to plan the read of all (search + select) columns. This is enhanced to plan and fetch only the
+ search columns. The rest of the stripe planning process optimizations remain unchanged e.g. partial read planning of
+ the stripe based on RowGroup statistics.
+* **Next Batch** identifies the processing that takes place when `RecordReader.nextBatch` is invoked.
+ * **Read Search Columns** takes place instead of reading all the selected columns. This is in sync with the planning
+ that has taken place during **Read Stripe** where only the search columns have been planned.
+ * **Apply Filter** on the batch that at this point only includes search columns. Evaluate the result of the filter:
+ * **Size = 0** indicates all records have been filtered out. Given this we proceed to the next batch of search
+ columns.
+ * **Size > 0** indicates that at least one record accepted by the filter. This record needs to be substantiated with
+ other columns.
+ * **Plan Select Columns** is invoked to perform read of the select columns. The planning happens as follows:
+ * Determine the current position of the read within the stripe and plan the read for the select columns from this
+ point forward to the end of the stripe.
+ * The Read planning of select columns respects the row groups filtered out as a result of the stripe planning.
+ * Fetch the select columns using the above plan.
+ * **Read Select Columns** into the vectorized row batch
+ * Return this batch.
+
+The current implementation performs a single read for the select columns in a stripe.
+
+```text
+┌──────────────────────────────────────────────────┐
+│ ┌────┐ ┌────┐ ┌────┐ ┌────┐ ┌────┐ ┌────┐ ┌────┐ │
+│ │RG0 │ │RG1 │ │RG2■│ │RG3 │ │RG4 │ │RG5■│ │RG6 │ │
+│ └────┘ └────┘ └────┘ └────┘ └────┘ └────┘ └────┘ │
+│ Stripe │
+└──────────────────────────────────────────────────┘
+```
+
+The above diagram depicts a stripe with 7 Row Groups out of which **RG2** and **RG5** are selected by the filter. The
+current implementation does the following:
+
+* Start the read planning process from the first match RG2
+* Read to the end of the stripe that includes RG6
+* Based on the above fetch skips RG0 and RG1 subject to compression block boundaries
+
+The above logic could be enhanced to perform say **2 or n** reads before reading to the end of stripe. The current
+implementation allows 0 reads before reading to the end of the stripe. The value of **n** could be configurable but
+should avoid too many short reads.
+
+The read behavior changes as follows with multiple reads being allowed within a stripe for select columns:
+
+```text
+┌──────────────────────────────────────────────────┐
+│ ┌────┐ ┌────┐ ┌────┐ ┌────┐ ┌────┐ ┌────┐ ┌────┐ │
+│ │ │ │ │ │■■■■│ │■■■■│ │■■■■│ │■■■■│ │■■■■│ │
+│ └────┘ └────┘ └────┘ └────┘ └────┘ └────┘ └────┘ │
+│ Current implementation │
+└──────────────────────────────────────────────────┘
+┌──────────────────────────────────────────────────┐
+│ ┌────┐ ┌────┐ ┌────┐ ┌────┐ ┌────┐ ┌────┐ ┌────┐ │
+│ │ │ │ │ │■■■■│ │ │ │ │ │■■■■│ │■■■■│ │
+│ └────┘ └────┘ └────┘ └────┘ └────┘ └────┘ └────┘ │
+│ Allow 1 partial read │
+└──────────────────────────────────────────────────┘
+```
+
+The figure shows that we could read significantly fewer bytes by performing an additional read before reading to the end
Review comment:
Why RG6 is still read? In addition, we may implement to merge small I/Os of selected RGs and avoid issuing I/O of unselected RGs at the best effort.
##########
File path: java/core/src/java/org/apache/orc/impl/reader/tree/StructBatchReader.java
##########
@@ -15,62 +15,80 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
+
package org.apache.orc.impl.reader.tree;
import org.apache.hadoop.hive.ql.exec.vector.ColumnVector;
import org.apache.hadoop.hive.ql.exec.vector.VectorizedRowBatch;
+import org.apache.orc.filter.OrcFilterContext;
import org.apache.orc.impl.TreeReaderFactory;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
import java.io.IOException;
-import java.util.Set;
public class StructBatchReader extends BatchReader {
+ private static final Logger LOG = LoggerFactory.getLogger(StructBatchReader.class);
// The reader context including row-filtering details
private final TreeReaderFactory.Context context;
+ private final TreeReaderFactory.StructTreeReader structReader;
+ private final OrcFilterContext fc;
- public StructBatchReader(TreeReaderFactory.StructTreeReader rowReader, TreeReaderFactory.Context context) {
+ public StructBatchReader(TypeReader rowReader, TreeReaderFactory.Context context) {
super(rowReader);
this.context = context;
+ this.fc = new OrcFilterContext(context.getSchemaEvolution().getReaderSchema());
+ if (rowReader instanceof TreeReaderFactory.StructTreeReader) {
+ structReader = (TreeReaderFactory.StructTreeReader) rowReader;
+ } else {
+ structReader = (TreeReaderFactory.StructTreeReader) ((LevelTypeReader)rowReader).getReader();
+ }
}
- private void readBatchColumn(VectorizedRowBatch batch, TypeReader[] children, int batchSize, int index)
- throws IOException {
+ private void readBatchColumn(VectorizedRowBatch batch,
+ TypeReader[] children,
+ int batchSize,
+ int index,
+ ReadLevel readLevel)
+ throws IOException {
ColumnVector colVector = batch.cols[index];
if (colVector != null) {
colVector.reset();
colVector.ensureSize(batchSize, false);
- children[index].nextVector(colVector, null, batchSize, batch);
+ children[index].nextVector(colVector, null, batchSize, batch, readLevel);
}
}
@Override
- public void nextBatch(VectorizedRowBatch batch, int batchSize) throws IOException {
- TypeReader[] children = ((TreeReaderFactory.StructTreeReader) rootType).fields;
- // Early expand fields --> apply filter --> expand remaining fields
- Set<Integer> earlyExpandCols = context.getColumnFilterIds();
+ public void nextBatch(VectorizedRowBatch batch, int batchSize, ReadLevel readLevel)
+ throws IOException {
+ nextBatchLevel(batch, batchSize, readLevel);
- // Clear selected and early expand columns used in Filter
- batch.selectedInUse = false;
- for (int i = 0; i < children.length && !earlyExpandCols.isEmpty() &&
- (vectorColumnCount == -1 || i < vectorColumnCount); ++i) {
- if (earlyExpandCols.contains(children[i].getColumnId())) {
- readBatchColumn(batch, children, batchSize, i);
+ if (readLevel == ReadLevel.LEAD) {
+ // Apply filter callback to reduce number of # rows selected for decoding in the next
+ // TreeReaders
+ if (this.context.getColumnFilterCallback() != null) {
+ this.context.getColumnFilterCallback().accept(fc.setBatch(batch));
}
}
- // Since we are going to filter rows based on some column values set batch.size earlier here
- batch.size = batchSize;
+ }
+
+ private void nextBatchLevel(VectorizedRowBatch batch, int batchSize, ReadLevel readLevel) throws IOException {
Review comment:
IMHO, prefer renaming it to nextBatchInternal.
##########
File path: java/core/src/java/org/apache/orc/impl/reader/tree/StructBatchReader.java
##########
@@ -15,62 +15,80 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
+
package org.apache.orc.impl.reader.tree;
import org.apache.hadoop.hive.ql.exec.vector.ColumnVector;
import org.apache.hadoop.hive.ql.exec.vector.VectorizedRowBatch;
+import org.apache.orc.filter.OrcFilterContext;
import org.apache.orc.impl.TreeReaderFactory;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
import java.io.IOException;
-import java.util.Set;
public class StructBatchReader extends BatchReader {
+ private static final Logger LOG = LoggerFactory.getLogger(StructBatchReader.class);
// The reader context including row-filtering details
private final TreeReaderFactory.Context context;
+ private final TreeReaderFactory.StructTreeReader structReader;
+ private final OrcFilterContext fc;
- public StructBatchReader(TreeReaderFactory.StructTreeReader rowReader, TreeReaderFactory.Context context) {
+ public StructBatchReader(TypeReader rowReader, TreeReaderFactory.Context context) {
super(rowReader);
this.context = context;
+ this.fc = new OrcFilterContext(context.getSchemaEvolution().getReaderSchema());
+ if (rowReader instanceof TreeReaderFactory.StructTreeReader) {
+ structReader = (TreeReaderFactory.StructTreeReader) rowReader;
+ } else {
+ structReader = (TreeReaderFactory.StructTreeReader) ((LevelTypeReader)rowReader).getReader();
+ }
}
- private void readBatchColumn(VectorizedRowBatch batch, TypeReader[] children, int batchSize, int index)
- throws IOException {
+ private void readBatchColumn(VectorizedRowBatch batch,
+ TypeReader[] children,
+ int batchSize,
+ int index,
+ ReadLevel readLevel)
+ throws IOException {
ColumnVector colVector = batch.cols[index];
if (colVector != null) {
colVector.reset();
colVector.ensureSize(batchSize, false);
- children[index].nextVector(colVector, null, batchSize, batch);
+ children[index].nextVector(colVector, null, batchSize, batch, readLevel);
}
}
@Override
- public void nextBatch(VectorizedRowBatch batch, int batchSize) throws IOException {
- TypeReader[] children = ((TreeReaderFactory.StructTreeReader) rootType).fields;
- // Early expand fields --> apply filter --> expand remaining fields
- Set<Integer> earlyExpandCols = context.getColumnFilterIds();
+ public void nextBatch(VectorizedRowBatch batch, int batchSize, ReadLevel readLevel)
+ throws IOException {
+ nextBatchLevel(batch, batchSize, readLevel);
- // Clear selected and early expand columns used in Filter
- batch.selectedInUse = false;
- for (int i = 0; i < children.length && !earlyExpandCols.isEmpty() &&
- (vectorColumnCount == -1 || i < vectorColumnCount); ++i) {
- if (earlyExpandCols.contains(children[i].getColumnId())) {
- readBatchColumn(batch, children, batchSize, i);
+ if (readLevel == ReadLevel.LEAD) {
+ // Apply filter callback to reduce number of # rows selected for decoding in the next
+ // TreeReaders
+ if (this.context.getColumnFilterCallback() != null) {
+ this.context.getColumnFilterCallback().accept(fc.setBatch(batch));
}
}
- // Since we are going to filter rows based on some column values set batch.size earlier here
- batch.size = batchSize;
+ }
+
+ private void nextBatchLevel(VectorizedRowBatch batch, int batchSize, ReadLevel readLevel) throws IOException {
+ TypeReader[] children = structReader.fields;
- // Apply filter callback to reduce number of # rows selected for decoding in the next TreeReaders
- if (!earlyExpandCols.isEmpty() && this.context.getColumnFilterCallback() != null) {
- this.context.getColumnFilterCallback().accept(batch);
+ if (readLevel != ReadLevel.FOLLOW) {
+ // In case of FOLLOW we leave the selectedInUse untouched.
+ batch.selectedInUse = false;
Review comment:
What does selectedInUse mean?
##########
File path: java/core/src/java/org/apache/orc/impl/RecordReaderImpl.java
##########
@@ -83,17 +85,21 @@
private final boolean[] fileIncluded;
private final long rowIndexStride;
private long rowInStripe = 0;
+ private long followRowInStripe = 0;
private int currentStripe = -1;
private long rowBaseInStripe = 0;
private long rowCountInStripe = 0;
private final BatchReader reader;
private final OrcIndex indexes;
+ private final boolean[] rowIndexCols;
private final SargApplier sargApp;
// an array about which row groups aren't skipped
private boolean[] includedRowGroups = null;
private final DataReader dataReader;
private final int maxDiskRangeChunkLimit;
private final StripePlanner planner;
+ private final ReadLevel readLevel;
+ private boolean needFollowStripe;
Review comment:
same as above
##########
File path: java/core/src/java/org/apache/orc/impl/RecordReaderImpl.java
##########
@@ -83,17 +85,21 @@
private final boolean[] fileIncluded;
private final long rowIndexStride;
private long rowInStripe = 0;
+ private long followRowInStripe = 0;
private int currentStripe = -1;
private long rowBaseInStripe = 0;
private long rowCountInStripe = 0;
private final BatchReader reader;
private final OrcIndex indexes;
+ private final boolean[] rowIndexCols;
Review comment:
better to add comment here as the name is not very clear
##########
File path: java/core/src/java/org/apache/orc/filter/OrcFilterContext.java
##########
@@ -0,0 +1,169 @@
+/*
+ * 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.orc.filter;
+
+import org.apache.hadoop.hive.ql.exec.vector.ColumnVector;
+import org.apache.hadoop.hive.ql.exec.vector.StructColumnVector;
+import org.apache.hadoop.hive.ql.exec.vector.VectorizedRowBatch;
+import org.apache.hadoop.hive.ql.io.filter.MutableFilterContext;
+import org.apache.orc.TypeDescription;
+import org.jetbrains.annotations.NotNull;
+
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.regex.Pattern;
+
+/**
+ * This defines the input for any filter operation.
+ * <p>
+ * It offers a convenience method for finding the column vector from a given name, that the filters
+ * can invoke to get access to the column vector.
+ */
+public class OrcFilterContext implements MutableFilterContext {
+
+ VectorizedRowBatch batch = null;
+ // Cache of field to ColumnVector, this is reset everytime the batch reference changes
+ private final Map<String, ColumnVector> vectors;
+ private final TypeDescription readSchema;
+
+ public OrcFilterContext(TypeDescription readSchema) {
+ this.readSchema = readSchema;
+ this.vectors = new HashMap<>();
+ }
+
+ public OrcFilterContext setBatch(@NotNull VectorizedRowBatch batch) {
+ if (batch != this.batch) {
+ this.batch = batch;
+ vectors.clear();
+ }
+ return this;
+ }
+
+ @Override
+ public void setFilterContext(boolean selectedInUse, int[] selected, int selectedSize) {
+ batch.setFilterContext(selectedInUse, selected, selectedSize);
+ }
+
+ @Override
+ public boolean validateSelected() {
+ return batch.validateSelected();
+ }
+
+ @Override
+ public int[] updateSelected(int i) {
+ return batch.updateSelected(i);
+ }
+
+ @Override
+ public void setSelectedInUse(boolean b) {
+ batch.setSelectedInUse(b);
+ }
+
+ @Override
+ public void setSelected(int[] ints) {
+ batch.setSelected(ints);
+ }
+
+ @Override
+ public void setSelectedSize(int i) {
+ batch.setSelectedSize(i);
+ }
+
+ @Override
+ public void reset() {
+ batch.reset();
+ }
+
+ @Override
+ public boolean isSelectedInUse() {
+ return batch.isSelectedInUse();
+ }
+
+ @Override
+ public int[] getSelected() {
+ return batch.getSelected();
+ }
+
+ @Override
+ public int getSelectedSize() {
+ return batch.getSelectedSize();
+ }
+
+ public ColumnVector[] getCols() {
+ return batch.cols;
+ }
+
+ /**
+ * Retrieves the column vector that matches the specified name. Allows support for nested struct
+ * references e.g. order.date where data is a field in a struct called order.
+ *
+ * @param name The column name whose vector should be retrieved
+ * @return The column vector
+ * @throws IllegalArgumentException if the field is not found or if the nested field is not part
+ * of a struct
+ */
+ public ColumnVector findColumnVector(String name) {
+ if (!vectors.containsKey(name)) {
+ vectors.put(name, findVector(name));
+ }
+
+ return vectors.get(name);
+ }
+
+ private ColumnVector findVector(String name) {
+ String[] refs = name.split(SPLIT_ON_PERIOD);
+ TypeDescription schema = readSchema;
+ List<String> fNames = schema.getFieldNames();
+ ColumnVector[] cols = batch.cols;
+ ColumnVector vector;
+
+ // Try to find the required column at the top level
+ int idx = findVectorIdx(refs[0], fNames);
+ vector = cols[idx];
+
+ // In case we are asking for a nested search further. Only nested struct references are allowed.
+ for (int i = 1; i < refs.length; i++) {
+ schema = schema.getChildren().get(idx);
+ if (schema.getCategory() != TypeDescription.Category.STRUCT) {
+ throw new IllegalArgumentException(String.format(
+ "Field %s:%s not a struct field does not allow nested reference",
+ refs[i - 1],
+ schema));
+ }
+ cols = ((StructColumnVector) vector).fields;
+ vector = cols[findVectorIdx(refs[i], schema.getFieldNames())];
+ }
+
+ return vector;
+ }
+
+ private static int findVectorIdx(String name, List<String> fieldNames) {
+ int idx = fieldNames.indexOf(name);
+ if (idx < 0) {
+ throw new IllegalArgumentException(String.format("Field %s not found in %s",
Review comment:
If we try to read a column which is added after the creation of the file, an IllegalArgumentException will be thrown? I am not sure how that kind of column is handled here.
##########
File path: site/develop/design/lazy_filter.md
##########
@@ -0,0 +1,352 @@
+* [Lazy Filter](#LazyFilter)
+ * [Background](#Background)
+ * [Design](#Design)
+ * [SArg to Filter](#SArgtoFilter)
+ * [Read](#Read)
+ * [Configuration](#Configuration)
+ * [Tests](#Tests)
+ * [Appendix](#Appendix)
+ * [Benchmarks](#Benchmarks)
+ * [Row vs Vector](#RowvsVector)
+ * [Filter](#Filter)
+
+# Lazy Filter <a id="LazyFilter"></a>
+
+## Background <a id="Background"></a>
+
+This feature request started as a result of a large search that is performed with the following characteristics:
+
+* The search fields are not part of partition, bucket or sort fields.
+* The table is a very large table.
+* The predicates result in very few rows compared to the scan size.
+* The search columns are a significant subset of selection columns in the query.
+
+Initial analysis showed that we could have a significant benefit by lazily reading the non-search columns only when we
+have a match. We explore the design and some benchmarks in subsequent sections.
+
+## Design <a id="Design"></a>
+
+This builds further on [ORC-577][ORC-577] which currently only restricts deserialization for some selected data types
+but does not improve on IO.
+
+On a high level the design includes the following components:
+
+```text
+┌──────────────┐ ┌────────────────────────┐
+│ │ │ Read │
+│ │ │ │
+│ │ │ ┌────────────┐ │
+│SArg to Filter│─────────▶│ │Read Filter │ │
+│ │ │ │ Columns │ │
+│ │ │ └────────────┘ │
+│ │ │ │ │
+└──────────────┘ │ ▼ │
+ │ ┌────────────┐ │
+ │ │Apply Filter│ │
+ │ └────────────┘ │
+ │ │ │
+ │ ▼ │
+ │ ┌────────────┐ │
+ │ │Read Select │ │
+ │ │ Columns │ │
+ │ └────────────┘ │
+ │ │
+ │ │
+ └────────────────────────┘
+```
+
+* **SArg to Filter**: Converts Search Arguments passed down into filters for efficient application during scans.
+* **Read**: Performs the lazy read using the filters.
+ * **Read Filter Columns**: Read the filter columns from the file.
+ * **Apply Filter**: Apply the filter on the read filter columns.
+ * **Read Select Columns**: If filter selects at least a row then read the remaining columns.
+
+### SArg to Filter <a id="SArgtoFilter"></a>
+
+SArg to Filter converts the passed SArg into a filter. This enables automatic compatibility with both Spark and Hive as
+they already push down Search Arguments down to ORC.
+
+The SArg is automatically converted into a [Vector Filter][vfilter]. Which is applied during the read process. Two
+filter types were evaluated:
+
+* [Row Filter][rfilter] that evaluates each row across all the predicates once.
+* [Vector Filter][vfilter] that evaluates each filter across the entire vector and adjusts the subsequent evaluation.
+
+While a row based filter is easier to code, it is much [slower][rowvvector] to process. We also see a significant
+[performance gain][rowvvector] in the absence of normalization.
+
+The builder for search argument should allow skipping normalization during the [build][build]. This has already been
+proposed as part of [HIVE-24458][HIVE-24458].
+
+### Read <a id="Read"></a>
+
+The read process has the following changes:
+
+```text
+ │
+ │
+ │
+┌────────────────────────▼────────────────────────┐
+│ ┏━━━━━━━━━━━━━━━━┓ │
+│ ┃Plan ++Search++ ┃ │
+│ ┃ Columns ┃ │
+│ ┗━━━━━━━━━━━━━━━━┛ │
+│ Read │Stripe │
+└────────────────────────┼────────────────────────┘
+ │
+ ▼
+
+
+ │
+ │
+┌────────────────────────▼────────────────────────┐
+│ ┏━━━━━━━━━━━━━━━━┓ │
+│ ┃Read ++Search++ ┃ │
+│ ┃ Columns ┃◀─────────┐ │
+│ ┗━━━━━━━━━━━━━━━━┛ │ │
+│ │ Size = 0 │
+│ ▼ │ │
+│ ┏━━━━━━━━━━━━━━━━┓ │ │
+│ ┃ Apply Filter ┃──────────┘ │
+│ ┗━━━━━━━━━━━━━━━━┛ │
+│ Size > 0 │
+│ │ │
+│ ▼ │
+│ ┏━━━━━━━━━━━━━━━━┓ │
+│ ┃ Plan Select ┃ │
+│ ┃ Columns ┃ │
+│ ┗━━━━━━━━━━━━━━━━┛ │
+│ │ │
+│ ▼ │
+│ ┏━━━━━━━━━━━━━━━━┓ │
+│ ┃ Read Select ┃ │
+│ ┃ Columns ┃ │
+│ ┗━━━━━━━━━━━━━━━━┛ │
+│ Next │Batch │
+└────────────────────────┼────────────────────────┘
+ │
+ ▼
+```
+
+The read process changes:
+
+* **Read Stripe** used to plan the read of all (search + select) columns. This is enhanced to plan and fetch only the
+ search columns. The rest of the stripe planning process optimizations remain unchanged e.g. partial read planning of
+ the stripe based on RowGroup statistics.
+* **Next Batch** identifies the processing that takes place when `RecordReader.nextBatch` is invoked.
+ * **Read Search Columns** takes place instead of reading all the selected columns. This is in sync with the planning
+ that has taken place during **Read Stripe** where only the search columns have been planned.
+ * **Apply Filter** on the batch that at this point only includes search columns. Evaluate the result of the filter:
+ * **Size = 0** indicates all records have been filtered out. Given this we proceed to the next batch of search
+ columns.
+ * **Size > 0** indicates that at least one record accepted by the filter. This record needs to be substantiated with
+ other columns.
+ * **Plan Select Columns** is invoked to perform read of the select columns. The planning happens as follows:
+ * Determine the current position of the read within the stripe and plan the read for the select columns from this
+ point forward to the end of the stripe.
+ * The Read planning of select columns respects the row groups filtered out as a result of the stripe planning.
+ * Fetch the select columns using the above plan.
+ * **Read Select Columns** into the vectorized row batch
+ * Return this batch.
+
+The current implementation performs a single read for the select columns in a stripe.
+
+```text
+┌──────────────────────────────────────────────────┐
+│ ┌────┐ ┌────┐ ┌────┐ ┌────┐ ┌────┐ ┌────┐ ┌────┐ │
+│ │RG0 │ │RG1 │ │RG2■│ │RG3 │ │RG4 │ │RG5■│ │RG6 │ │
+│ └────┘ └────┘ └────┘ └────┘ └────┘ └────┘ └────┘ │
+│ Stripe │
+└──────────────────────────────────────────────────┘
+```
+
+The above diagram depicts a stripe with 7 Row Groups out of which **RG2** and **RG5** are selected by the filter. The
+current implementation does the following:
+
+* Start the read planning process from the first match RG2
+* Read to the end of the stripe that includes RG6
+* Based on the above fetch skips RG0 and RG1 subject to compression block boundaries
+
+The above logic could be enhanced to perform say **2 or n** reads before reading to the end of stripe. The current
+implementation allows 0 reads before reading to the end of the stripe. The value of **n** could be configurable but
+should avoid too many short reads.
+
+The read behavior changes as follows with multiple reads being allowed within a stripe for select columns:
+
+```text
+┌──────────────────────────────────────────────────┐
+│ ┌────┐ ┌────┐ ┌────┐ ┌────┐ ┌────┐ ┌────┐ ┌────┐ │
+│ │ │ │ │ │■■■■│ │■■■■│ │■■■■│ │■■■■│ │■■■■│ │
+│ └────┘ └────┘ └────┘ └────┘ └────┘ └────┘ └────┘ │
+│ Current implementation │
+└──────────────────────────────────────────────────┘
+┌──────────────────────────────────────────────────┐
+│ ┌────┐ ┌────┐ ┌────┐ ┌────┐ ┌────┐ ┌────┐ ┌────┐ │
+│ │ │ │ │ │■■■■│ │ │ │ │ │■■■■│ │■■■■│ │
+│ └────┘ └────┘ └────┘ └────┘ └────┘ └────┘ └────┘ │
+│ Allow 1 partial read │
+└──────────────────────────────────────────────────┘
+```
+
+The figure shows that we could read significantly fewer bytes by performing an additional read before reading to the end
+of stripe. This shall be included as a subsequent enhancement to this patch.
+
+## Configuration <a id="Configuration"></a>
+
+The following configuration options are exposed that control the filter behavior:
+
+|Property |Type |Default|
+|:--- |:--- |:--- |
+|orc.sarg.to.filter |boolean|false |
+|orc.sarg.to.filter.selected|boolean|false |
+
+* `orc.sarg.to.filter` can be used to turn off the SArg to filter conversion. This might be particularly relevant in
+ cases where the filter is expensive and does not eliminate a lot of records.
+* `orc.sarg.to.filter.selected` is an important setting that if incorrectly enabled results in wrong output. The
+ `VectorizedRowBatch` has a selected vector that defines which rows are selected. This property should be set to `true`
+ only if the consumer respects the selected vector in determining the valid rows.
+
+## Tests <a id="Tests"></a>
+
+We evaluated this patch against a search job with the following stats:
+
+* Table
+ * Size: ~**420 TB**
+ * Data fields: ~**120**
+ * Partition fields: **3**
+* Scan
+ * Search fields: 3 data fields with large (~ 1000 value) IN clauses compounded by **OR**.
+ * Select fields: 16 data fields (includes the 3 search fields), 1 partition field
+ * Search:
+ * Size: ~**180 TB**
+ * Records: **3.99 T**
+ * Selected:
+ * Size: ~**100 MB**
+ * Records: **1 M**
+
+We have observed the following reductions:
+
+|Test |IO Reduction %|CPU Reduction %|
+|:--- | ---:| ---:|
+|Same | 45| 47|
+|SELECT *| 70| 87|
+
+* The savings are more significant as you increase the number of select columns with respect to the search columns
+* When the filter selects most data, no significant penalty observed as a result of 2 IO compared with a single IO
+ * We do have a penalty as a result of the filter application on the selected records.
+
+## Appendix <a id="Appendix"></a>
+
+### Benchmarks <a id="Benchmarks"></a>
+
+#### Row vs Vector <a id="RowvsVector"></a>
+
+We start with a decision of using a Row filter vs a Vector filter. The Row filter has the advantage of simpler code vs
+the Vector filter.
+
+```bash
+java -jar target/orc-benchmarks-core-1.7.0-SNAPSHOT-uber.jar filter simple
+```
+
+|Benchmark |(fInSize)|(fType)|Mode| Cnt| Score|Error |Units|
+|:--- | ---:|:--- |:---|---:| ---:|:--- |:--- |
+|SimpleFilterBench.filter| 4|row |avgt| 20|52.260|± 0.109|us/op|
+|SimpleFilterBench.filter| 4|vector |avgt| 20|19.699|± 0.044|us/op|
+|SimpleFilterBench.filter| 8|row |avgt| 20|59.648|± 0.179|us/op|
+|SimpleFilterBench.filter| 8|vector |avgt| 20|28.655|± 0.036|us/op|
+|SimpleFilterBench.filter| 16|row |avgt| 20|56.480|± 0.190|us/op|
+|SimpleFilterBench.filter| 16|vector |avgt| 20|46.757|± 0.124|us/op|
+|SimpleFilterBench.filter| 32|row |avgt| 20|57.780|± 0.111|us/op|
+|SimpleFilterBench.filter| 32|vector |avgt| 20|52.060|± 0.333|us/op|
+|SimpleFilterBench.filter| 256|row |avgt| 20|50.898|± 0.275|us/op|
+|SimpleFilterBench.filter| 256|vector |avgt| 20|85.684|± 0.351|us/op|
+
+Explanation:
+
+* **fInSize** calls out the number of values in the IN clause.
+* **fType** calls out the whether the filter is a row based filter, or a vector based filter.
+
+Observations:
+
+* The vector based filter is significantly faster than the row based filter.
+ * At best, vector was faster by **59.62%**
+ * At worst, vector was faster by **32.14%**
+* The performance of the filters is deteriorates with the increase of the IN values, however even in this case the
+ vector filter is much better than the row filter.
+
+In the next test we use a complex filter with both AND, and OR to understand the impact of Conjunctive Normal Form on
Review comment:
Just curious whether PPD has a similar performance penality to normalize SArg to CNF.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
pavibhai commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r574102411
##########
File path: java/mapreduce/src/java/org/apache/orc/mapred/OrcMapredRecordReader.java
##########
@@ -98,16 +101,22 @@ public boolean next(NullWritable key, V value) throws IOException {
if (!ensureBatch()) {
return false;
}
+ int rowIdx = batch.selectedInUse ? batch.selected[rowInBatch] : rowInBatch;
if (schema.getCategory() == TypeDescription.Category.STRUCT) {
OrcStruct result = (OrcStruct) value;
List<TypeDescription> children = schema.getChildren();
int numberOfChildren = children.size();
for(int i=0; i < numberOfChildren; ++i) {
- result.setFieldValue(i, nextValue(batch.cols[i], rowInBatch,
- children.get(i), result.getFieldValue(i)));
+ TypeDescription child = children.get(i);
+ if (included == null || included[child.getId()]) {
+ result.setFieldValue(i, nextValue(batch.cols[i], rowIdx, child,
+ result.getFieldValue(i)));
+ } else {
+ result.setFieldValue(i, null);
+ }
Review comment:
Yes, we could. I need some guidance now on which changes can be combined or not.
So far we seem to have the following changes that are requested that we pull out separately:
1. Code changes like making a variable final or using the UTF8 Character Set
2. Changes to MapReduce classes
3. Findbugs exclude update
Can I create one PR that includes all of this or do you want separate PRs for each item that you requested a separate PR for?
I will anyways do a separate PR for the sliceAndShift change.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] dongjoon-hyun commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r566569325
##########
File path: java/mapreduce/src/java/org/apache/orc/mapred/OrcMapredRecordReader.java
##########
@@ -98,16 +101,22 @@ public boolean next(NullWritable key, V value) throws IOException {
if (!ensureBatch()) {
return false;
}
+ int rowIdx = batch.selectedInUse ? batch.selected[rowInBatch] : rowInBatch;
if (schema.getCategory() == TypeDescription.Category.STRUCT) {
OrcStruct result = (OrcStruct) value;
List<TypeDescription> children = schema.getChildren();
int numberOfChildren = children.size();
for(int i=0; i < numberOfChildren; ++i) {
- result.setFieldValue(i, nextValue(batch.cols[i], rowInBatch,
- children.get(i), result.getFieldValue(i)));
+ TypeDescription child = children.get(i);
+ if (included == null || included[child.getId()]) {
+ result.setFieldValue(i, nextValue(batch.cols[i], rowIdx, child,
+ result.getFieldValue(i)));
+ } else {
+ result.setFieldValue(i, null);
+ }
Review comment:
Do you think we can proceed `OrcMapredRecordReader` and `OrcMapreduceRecordReader` separately first?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
pavibhai commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r578824454
##########
File path: java/core/src/java/org/apache/orc/impl/RecordReaderImpl.java
##########
@@ -1170,6 +1189,10 @@ private void advanceStripe() throws IOException {
}
}
+ private int computeRGIdx(long rowIdx) {
Review comment:
added
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
pavibhai commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r578812099
##########
File path: java/core/src/java/org/apache/orc/impl/TreeReaderFactory.java
##########
@@ -2833,8 +2904,7 @@ public static TypeReader createTreeReader(TypeDescription readerType,
case DATE:
return new DateTreeReader(fileType.getId(), context);
case DECIMAL:
- if (version == OrcFile.Version.UNSTABLE_PRE_2_0 &&
- fileType.getPrecision() <= TypeDescription.MAX_DECIMAL64_PRECISION){
+ if (isDecimalAsLong(version, fileType.getPrecision())){
Review comment:
This is used in #636 I will remove from this PR and include it as part of that PR.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
pavibhai commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r600000971
##########
File path: java/core/src/java/org/apache/orc/impl/reader/tree/LevelStructBatchReader.java
##########
@@ -0,0 +1,28 @@
+/*
+ * 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.orc.impl.reader.tree;
+
+import org.apache.orc.impl.TreeReaderFactory;
+
+public class LevelStructBatchReader extends StructBatchReader {
Review comment:
This class has been removed. the logic moves into Struct and Union TreeReaders instead.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] dongjoon-hyun commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r565706319
##########
File path: java/core/src/java/org/apache/orc/impl/TreeReaderFactory.java
##########
@@ -195,6 +198,8 @@ public boolean fileUsedProlepticGregorian() {
protected BitFieldReader present = null;
protected final Context context;
+ protected final ReadLevel rLevel;
Review comment:
`rLevel` -> `readLevel`?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pgaref commented on pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
pgaref commented on pull request #635:
URL: https://github.com/apache/orc/pull/635#issuecomment-781509748
> > Left some initial comments here and there but in general I will have to agree with the existing comments that this is just to hard to review this PR and needs splitting. I would propose the following split:
>
> Thanks for your feedback and suggestions @pgaref
>
> >
>
> I would like to propose updates to checkstyle based on line and indentation feedback I have received on this PR.
>
> > ```
> > * Findbugs onliner
> > ```
>
> Can you please clarify what you mean by this?
>
> > ```
> > * Changes to MapReduce classes
> >
> > * Introduce OrcFilterContext (also affects row-filter tests etc. but is manageable)
> >
> > * Introduce ReadLevel on Reader and Planner
> >
> > * Introduce [ORC-743](https://issues.apache.org/jira/browse/ORC-743) changes to the planner (partial-reads?)
> > ```
>
> [ORC-743](https://issues.apache.org/jira/browse/ORC-743) includes SArg to Filter conversion, the partial reads are part of this PR. The configuration for partial reads is absent in both the PRs right now.
>
> I will treat this as introduce partial reads, followed by [ORC-743](https://issues.apache.org/jira/browse/ORC-743) which might have other splits that it needs.
>
> > ```
> > * Documentation changes after we merge all the expected functionality
> >
> > * Code changes like making a variable final or using the UTF8 Character Set, or new methods (less critical)
> > ```
>
> Any changes that are not required we can pull it out to an unrelated PR. I would like to include methods that avoid code duplication into the PR still.
>
> > Please let me know what you think and again thanks for all the work!
>
> Hope that makes sense. Please let me know.
> > * Findbugs onliner
What I meant:
https://github.com/apache/orc/pull/635/files/4f400a3893aa7d7a32ffad7970041a6e8acf7c28#diff-e1aaa921b730eea0b914cb64c8adfe6e743a0395c114d60f26ee19e036da20e9
>I will treat this as introduce partial reads,
Sounds good to me -- lets just make it configurable
> I would like to include methods that avoid code duplication into the PR still.
Sure, it makes sense
@pavibhai Sounds like a plan -- lets start splitting this and I will be happy to help along the way!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
pavibhai commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r589647534
##########
File path: java/core/src/java/org/apache/orc/filter/OrcFilterContext.java
##########
@@ -0,0 +1,169 @@
+/*
+ * 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.orc.filter;
+
+import org.apache.hadoop.hive.ql.exec.vector.ColumnVector;
+import org.apache.hadoop.hive.ql.exec.vector.StructColumnVector;
+import org.apache.hadoop.hive.ql.exec.vector.VectorizedRowBatch;
+import org.apache.hadoop.hive.ql.io.filter.MutableFilterContext;
+import org.apache.orc.TypeDescription;
+import org.jetbrains.annotations.NotNull;
+
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.regex.Pattern;
+
+/**
+ * This defines the input for any filter operation.
+ * <p>
+ * It offers a convenience method for finding the column vector from a given name, that the filters
+ * can invoke to get access to the column vector.
+ */
+public class OrcFilterContext implements MutableFilterContext {
Review comment:
Merged as part of ORC-755
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
pavibhai commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r574100331
##########
File path: site/develop/design/lazy_filter.md
##########
@@ -0,0 +1,352 @@
+* [Lazy Filter](#LazyFilter)
Review comment:
Sure will pull this out into a separate PR
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
pavibhai commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r589648302
##########
File path: java/core/src/java/org/apache/orc/impl/RecordReaderImpl.java
##########
@@ -748,11 +763,11 @@ private static TruthValue checkInBloomFilter(BloomFilter bf,
TruthValue result = hasNull ? TruthValue.NO_NULL : TruthValue.NO;
if (predObj instanceof Long) {
- if (bf.testLong(((Long) predObj).longValue())) {
+ if (bf.testLong((Long) predObj)) {
Review comment:
Merged as part of ORC-754
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
pavibhai commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r578814294
##########
File path: java/core/src/java/org/apache/orc/impl/RecordReaderImpl.java
##########
@@ -748,11 +763,11 @@ private static TruthValue checkInBloomFilter(BloomFilter bf,
TruthValue result = hasNull ? TruthValue.NO_NULL : TruthValue.NO;
if (predObj instanceof Long) {
- if (bf.testLong(((Long) predObj).longValue())) {
+ if (bf.testLong((Long) predObj)) {
Review comment:
yeah not required for this patch, will move to separate PR
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
pavibhai commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r566065109
##########
File path: .gitignore
##########
@@ -9,3 +9,4 @@ target
*.iws
.idea
.DS_Store
+*.monopic
Review comment:
Removed
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] pavibhai commented on a change in pull request #635: ORC-742: LazyIO for non-filter columns
Posted by GitBox <gi...@apache.org>.
pavibhai commented on a change in pull request #635:
URL: https://github.com/apache/orc/pull/635#discussion_r589640813
##########
File path: java/core/src/java/org/apache/orc/impl/TreeReaderFactory.java
##########
@@ -67,7 +70,7 @@
Set<Integer> getColumnFilterIds();
- Consumer<VectorizedRowBatch> getColumnFilterCallback();
+ Consumer<org.apache.orc.filter.FilterContext> getColumnFilterCallback();
Review comment:
Merged as part of ORC-755
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org