You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@hive.apache.org by GitBox <gi...@apache.org> on 2022/08/30 22:37:33 UTC
[GitHub] [hive] difin opened a new pull request, #3559: HIVE-26496,Improvement to fetch operator to scan only delete_delta fo…
difin opened a new pull request, #3559:
URL: https://github.com/apache/hive/pull/3559
…lders containing transactions relevant to the currently processed split.
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://cwiki.apache.org/confluence/display/Hive/HowToContribute
2. Ensure that you have created an issue on the Hive project JIRA: https://issues.apache.org/jira/projects/HIVE/summary
3. Ensure you have added or run the appropriate tests for your PR:
4. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP]HIVE-XXXXX: Your PR title ...'.
5. Be sure to keep the PR description updated to reflect all changes.
6. Please write your PR title to summarize what this PR proposes.
7. If possible, provide a concise example to reproduce the issue for a faster review.
-->
### What changes were proposed in this pull request?
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
The proposed changes improve performance of FetchOperator.
Currently, when a table has a list of delta and delete_delta folders, for every split the system is scanning for delete events in all delete_delta folders. But this filtering is not needed for delete_delta folders which only have events that happened in the past relative to the current split.
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
As reporting in HIVE-26496, for a table having 21 delta and delete_delta folders the unnecessary scanning of all delete_delta folders for every split makes select query executions super slow, even when there is minimal dataset present in the table.
### Does this PR introduce _any_ user-facing change?
<!--
Note that it means *any* user-facing change including all aspects such as the documentation fix.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description, screenshot and/or a reproducable example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Hive versions or within the unreleased branches such as master.
If no, write 'No'.
-->
No
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
-->
Execution of unit and integration tests.
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] abstractdog commented on pull request #3559: HIVE-26496,Improvement to fetch operator to scan only delete_delta fo…
Posted by GitBox <gi...@apache.org>.
abstractdog commented on PR #3559:
URL: https://github.com/apache/hive/pull/3559#issuecomment-1246530870
left minor comments, but basically looks good to me, I'm tempted to approve this immediately
just wondering if anyone with ACID background can see any obvious problems: @deniskuzZ , @lcspinter
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3559: HIVE-26496,Improvement to fetch operator to scan only delete_delta fo…
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3559:
URL: https://github.com/apache/hive/pull/3559#discussion_r971049370
##########
ql/src/java/org/apache/hadoop/hive/ql/io/orc/OrcSplit.java:
##########
@@ -104,28 +104,43 @@ public OrcSplit(Path path, Object fileId, long offset, long length, String[] hos
this.isOriginal = isOriginal;
this.hasBase = hasBase;
this.rootDir = rootDir;
- this.deltas.addAll(filterDeltasByBucketId(deltas, AcidUtils.parseBucketId(path)));
+ int bucketId = AcidUtils.parseBucketId(path);
+ AcidUtils.ParsedDeltaLight parentDelta = AcidUtils.ParsedDeltaLight.parse(getPath().getParent());
Review Comment:
could be refactored using static import:
````
ParsedDeltaLight pd = ParsedDeltaLight.parse(path.getParent())
````
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] difin commented on a diff in pull request #3559: HIVE-26496,Improvement to fetch operator to scan only delete_delta fo…
Posted by GitBox <gi...@apache.org>.
difin commented on code in PR #3559:
URL: https://github.com/apache/hive/pull/3559#discussion_r974613822
##########
ql/src/java/org/apache/hadoop/hive/ql/io/orc/OrcSplit.java:
##########
@@ -104,7 +103,15 @@ public OrcSplit(Path path, Object fileId, long offset, long length, String[] hos
this.isOriginal = isOriginal;
this.hasBase = hasBase;
this.rootDir = rootDir;
- this.deltas.addAll(filterDeltasByBucketId(deltas, AcidUtils.parseBucketId(path)));
+ int bucketId = AcidUtils.parseBucketId(path);
Review Comment:
Hi @deniskuzZ @zabetak
Fixed in PR https://github.com/apache/hive/pull/3606.
Can you please review the new PR and merge, if you find it is good?
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] difin commented on a diff in pull request #3559: HIVE-26496,Improvement to fetch operator to scan only delete_delta fo…
Posted by GitBox <gi...@apache.org>.
difin commented on code in PR #3559:
URL: https://github.com/apache/hive/pull/3559#discussion_r971190432
##########
ql/src/java/org/apache/hadoop/hive/ql/io/orc/OrcSplit.java:
##########
@@ -104,28 +104,43 @@ public OrcSplit(Path path, Object fileId, long offset, long length, String[] hos
this.isOriginal = isOriginal;
this.hasBase = hasBase;
this.rootDir = rootDir;
- this.deltas.addAll(filterDeltasByBucketId(deltas, AcidUtils.parseBucketId(path)));
+ int bucketId = AcidUtils.parseBucketId(path);
+ AcidUtils.ParsedDeltaLight parentDelta = AcidUtils.ParsedDeltaLight.parse(getPath().getParent());
Review Comment:
Hi @deniskuzZ,
Many tests failed with the change of using AcidUtils.ParsedDeltaLight.parse() instead of AcidUtils.parseBaseOrDeltaBucketFilename(). As I understand the split is not always a delta folder, it can be some older format not supported by ParsedDeltaLight. I saw that ParsedDeltaLight.parse() is used in some cases internally in AcidUtils.parseBaseOrDeltaBucketFilename(), but not always. Can you please advise if I should revert to using AcidUtils.parseBaseOrDeltaBucketFilename() that worked in all cases or there is some better way?
(https://github.com/apache/hive/blob/master/ql/src/java/org/apache/hadoop/hive/ql/io/AcidUtils.java#539)
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] difin commented on a diff in pull request #3559: HIVE-26496,Improvement to fetch operator to scan only delete_delta fo…
Posted by GitBox <gi...@apache.org>.
difin commented on code in PR #3559:
URL: https://github.com/apache/hive/pull/3559#discussion_r971190432
##########
ql/src/java/org/apache/hadoop/hive/ql/io/orc/OrcSplit.java:
##########
@@ -104,28 +104,43 @@ public OrcSplit(Path path, Object fileId, long offset, long length, String[] hos
this.isOriginal = isOriginal;
this.hasBase = hasBase;
this.rootDir = rootDir;
- this.deltas.addAll(filterDeltasByBucketId(deltas, AcidUtils.parseBucketId(path)));
+ int bucketId = AcidUtils.parseBucketId(path);
+ AcidUtils.ParsedDeltaLight parentDelta = AcidUtils.ParsedDeltaLight.parse(getPath().getParent());
Review Comment:
Hi @deniskuzZ,
Many tests failed with the change of using AcidUtils.ParsedDeltaLight.parse() instead of AcidUtils.parseBaseOrDeltaBucketFilename(). As I understand the split is not always a delta folder, it can be some older format not supported by ParsedDeltaLight. I saw that ParsedDeltaLight.parse() is used in some cases internally in AcidUtils.parseBaseOrDeltaBucketFilename(), but not always. Can you please advise if I should revert to using AcidUtils.parseBaseOrDeltaBucketFilename() that worked in all cases or there is some better 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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3559: HIVE-26496,Improvement to fetch operator to scan only delete_delta fo…
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3559:
URL: https://github.com/apache/hive/pull/3559#discussion_r973296993
##########
ql/src/java/org/apache/hadoop/hive/ql/io/orc/OrcSplit.java:
##########
@@ -104,7 +103,15 @@ public OrcSplit(Path path, Object fileId, long offset, long length, String[] hos
this.isOriginal = isOriginal;
this.hasBase = hasBase;
this.rootDir = rootDir;
- this.deltas.addAll(filterDeltasByBucketId(deltas, AcidUtils.parseBucketId(path)));
+ int bucketId = AcidUtils.parseBucketId(path);
Review Comment:
@zabetak nice catch, I didn't notice that there is an instance variable`bucketId` cause it wasn't initialized in a constructor, but in a parse() method. The whole thing needs to be refactored.
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] abstractdog commented on pull request #3559: HIVE-26496,Improvement to fetch operator to scan only delete_delta fo…
Posted by GitBox <gi...@apache.org>.
abstractdog commented on PR #3559:
URL: https://github.com/apache/hive/pull/3559#issuecomment-1245545220
could this be unit tested similarly to TestVectorizedOrcAcidRowBatchReader.testDeleteEventFiltering
https://github.com/apache/hive/blob/35f6a75c86e7b6df7460a046ce53e87a50d64860/ql/src/test/org/apache/hadoop/hive/ql/io/orc/TestVectorizedOrcAcidRowBatchReader.java#L228?
deleting some rows, generating splits, check getDeltas()?
let me know if it doesn't make sense, I'm not an ACID expert
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] sonarcloud[bot] commented on pull request #3559: HIVE-26496,Improvement to fetch operator to scan only delete_delta fo…
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #3559:
URL: https://github.com/apache/hive/pull/3559#issuecomment-1232271404
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=3559)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3559&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3559&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3559&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3559&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3559&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3559&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3559&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3559&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3559&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3559&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3559&resolved=false&types=CODE_SMELL) [2 Code Smells](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3559&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=3559&metric=coverage&view=list) No Coverage information
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=3559&metric=duplicated_lines_density&view=list) No Duplication information
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3559: HIVE-26496,Improvement to fetch operator to scan only delete_delta fo…
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3559:
URL: https://github.com/apache/hive/pull/3559#discussion_r959380862
##########
ql/src/java/org/apache/hadoop/hive/ql/io/orc/VectorizedOrcAcidRowBatchReader.java:
##########
@@ -885,7 +886,20 @@ static Path[] getDeleteDeltaDirsFromSplit(OrcSplit orcSplit,
} else {
throw new IllegalStateException("Split w/o base w/Acid 2.0??: " + path);
}
- return AcidUtils.deserializeDeleteDeltas(root, orcSplit.getDeltas(), pathToDeltaMetaData);
+
+ // filtering out delete_delta folders containing transactions that happened only before
+ // transactions of the split
+ List<AcidInputFormat.DeltaMetaData> deltas = orcSplit.getDeltas();
Review Comment:
wouldn't it be better if orcSplit.getDeltas() would already return only the relevant delete deltas?
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] difin commented on a diff in pull request #3559: HIVE-26496,Improvement to fetch operator to scan only delete_delta fo…
Posted by GitBox <gi...@apache.org>.
difin commented on code in PR #3559:
URL: https://github.com/apache/hive/pull/3559#discussion_r970928460
##########
ql/src/test/org/apache/hadoop/hive/ql/io/orc/TestOrcSplit.java:
##########
@@ -0,0 +1,181 @@
+/*
+ * 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.hadoop.hive.ql.io.orc;
+
+import org.apache.hadoop.fs.FileSystem;
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.hive.common.ValidReadTxnList;
+import org.apache.hadoop.hive.common.ValidTxnList;
+import org.apache.hadoop.hive.common.ValidWriteIdList;
+import org.apache.hadoop.hive.conf.HiveConf;
+import org.apache.hadoop.hive.metastore.api.hive_metastoreConstants;
+import org.apache.hadoop.hive.ql.io.*;
+import org.apache.hadoop.hive.ql.io.AcidUtils.Directory;
+import org.apache.hadoop.hive.serde2.objectinspector.ObjectInspector;
+import org.apache.hadoop.hive.serde2.objectinspector.ObjectInspectorFactory;
+import org.apache.hadoop.io.LongWritable;
+import org.apache.hadoop.mapred.JobConf;
+import org.apache.hadoop.mapred.Reporter;
+import org.apache.orc.OrcConf;
+import org.junit.Before;
+import org.junit.Test;
+
+import java.io.File;
+import java.util.*;
+
+import static org.junit.Assert.*;
+
+/**
+ * Tests for OrcSplit class
+ */
+public class TestOrcSplit {
+
+ private JobConf conf;
+ private FileSystem fs;
+ private Path root;
+ private ObjectInspector inspector;
+ public static class DummyRow {
+ LongWritable field;
+ RecordIdentifier ROW__ID;
+
+ DummyRow(long val, long rowId, long origTxn, int bucket) {
+ field = new LongWritable(val);
+ bucket = BucketCodec.V1.encode(new AcidOutputFormat.Options(null).bucket(bucket));
+ ROW__ID = new RecordIdentifier(origTxn, bucket, rowId);
+ }
+
+ static String getColumnNamesProperty() {
+ return "field";
+ }
+ static String getColumnTypesProperty() {
+ return "bigint";
+ }
+
+ }
+
+ @Before
+ public void setup() throws Exception {
+ conf = new JobConf();
+ conf.set(hive_metastoreConstants.TABLE_IS_TRANSACTIONAL, "true");
+ conf.setBoolean(HiveConf.ConfVars.HIVE_TRANSACTIONAL_TABLE_SCAN.varname, true);
+ conf.set(hive_metastoreConstants.TABLE_TRANSACTIONAL_PROPERTIES, "default");
+ conf.setInt(HiveConf.ConfVars.HIVE_TXN_OPERATIONAL_PROPERTIES.varname,
+ AcidUtils.AcidOperationalProperties.getDefault().toInt());
+ conf.set(IOConstants.SCHEMA_EVOLUTION_COLUMNS, DummyRow.getColumnNamesProperty());
+ conf.set(IOConstants.SCHEMA_EVOLUTION_COLUMNS_TYPES, DummyRow.getColumnTypesProperty());
+ conf.setBoolean(HiveConf.ConfVars.HIVE_VECTORIZATION_ENABLED.varname, true);
+ conf.set(HiveConf.ConfVars.HIVE_ORC_SPLIT_STRATEGY.varname, "BI");
+ OrcConf.ROWS_BETWEEN_CHECKS.setLong(conf, 1);
+
+ Path workDir = new Path(System.getProperty("test.tmp.dir",
+ "target" + File.separator + "test" + File.separator + "tmp"));
+ root = new Path(workDir, "TestOrcSplit.testDump");
+ fs = root.getFileSystem(conf);
+ root = fs.makeQualified(root);
+ fs.delete(root, true);
+ synchronized (TestOrcFile.class) {
+ inspector = ObjectInspectorFactory.getReflectionObjectInspector
+ (DummyRow.class, ObjectInspectorFactory.ObjectInspectorOptions.JAVA);
+ }
+ }
+
+ private List<OrcInputFormat.SplitStrategy<?>> getSplitStrategies() throws Exception {
+ conf.setInt(HiveConf.ConfVars.HIVE_TXN_OPERATIONAL_PROPERTIES.varname,
+ AcidUtils.AcidOperationalProperties.getDefault().toInt());
+ OrcInputFormat.Context context = new OrcInputFormat.Context(conf);
+ OrcInputFormat.FileGenerator gen = new OrcInputFormat.FileGenerator(
+ context, () -> fs, root, false, null);
+ Directory adi = gen.call();
+ return OrcInputFormat.determineSplitStrategies(
+ null, context, adi.getFs(), adi.getPath(), adi.getFiles(), adi.getDeleteDeltas(),
+ null, null, true);
+ }
+
+ /**
+ * This test checks that a split filters out delete_delta folders which only have transactions that happened
+ * in the past relative to the current split
+ */
+ @Test
+ public void testDeleteDeltasFiltering() throws Exception {
+
+ int bucket = 0;
+ AcidOutputFormat.Options options = new AcidOutputFormat.Options(conf)
+ .filesystem(fs)
+ .bucket(bucket)
+ .writingBase(false)
+ .minimumWriteId(1)
+ .maximumWriteId(1)
+ .inspector(inspector)
+ .reporter(Reporter.NULL)
+ .recordIdColumn(1)
+ .finalDestination(root);
+
+ RecordUpdater updater = new OrcRecordUpdater(root, options);
+
+ // Inserting a new record and then deleting it, 3 times.
+ // Every insertion/deletion is in a separate transaction.
+ // When reading, whis will generate 3 splits, one per insert event.
Review Comment:
fixed typo
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] difin commented on a diff in pull request #3559: HIVE-26496,Improvement to fetch operator to scan only delete_delta fo…
Posted by GitBox <gi...@apache.org>.
difin commented on code in PR #3559:
URL: https://github.com/apache/hive/pull/3559#discussion_r971944863
##########
ql/src/java/org/apache/hadoop/hive/ql/io/orc/OrcSplit.java:
##########
@@ -94,7 +93,7 @@ protected OrcSplit() {
public OrcSplit(Path path, Object fileId, long offset, long length, String[] hosts,
OrcTail orcTail, boolean isOriginal, boolean hasBase,
List<AcidInputFormat.DeltaMetaData> deltas, long projectedDataSize, long fileLen, Path rootDir,
- OffsetAndBucketProperty syntheticAcidProps) {
+ OffsetAndBucketProperty syntheticAcidProps) 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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] difin commented on a diff in pull request #3559: HIVE-26496,Improvement to fetch operator to scan only delete_delta fo…
Posted by GitBox <gi...@apache.org>.
difin commented on code in PR #3559:
URL: https://github.com/apache/hive/pull/3559#discussion_r971391049
##########
ql/src/java/org/apache/hadoop/hive/ql/io/orc/OrcSplit.java:
##########
@@ -104,28 +104,43 @@ public OrcSplit(Path path, Object fileId, long offset, long length, String[] hos
this.isOriginal = isOriginal;
this.hasBase = hasBase;
this.rootDir = rootDir;
- this.deltas.addAll(filterDeltasByBucketId(deltas, AcidUtils.parseBucketId(path)));
+ int bucketId = AcidUtils.parseBucketId(path);
+ AcidUtils.ParsedDeltaLight parentDelta = AcidUtils.ParsedDeltaLight.parse(getPath().getParent());
Review Comment:
reverted to previous version without ParsedDeltaLight.parse(path.getParent())
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3559: HIVE-26496,Improvement to fetch operator to scan only delete_delta fo…
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3559:
URL: https://github.com/apache/hive/pull/3559#discussion_r970992776
##########
ql/src/java/org/apache/hadoop/hive/ql/io/orc/OrcSplit.java:
##########
@@ -104,13 +104,32 @@ public OrcSplit(Path path, Object fileId, long offset, long length, String[] hos
this.isOriginal = isOriginal;
this.hasBase = hasBase;
this.rootDir = rootDir;
- this.deltas.addAll(filterDeltasByBucketId(deltas, AcidUtils.parseBucketId(path)));
+ this.deltas.addAll(filterDeleteDeltasByWriteIds
Review Comment:
looks good!
1 question, should we call addAll, or can we simply assign the result of the collect?
````
this.deltas = ....
collect(Collectors.toList());
````
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] sonarcloud[bot] commented on pull request #3559: HIVE-26496,Improvement to fetch operator to scan only delete_delta fo…
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #3559:
URL: https://github.com/apache/hive/pull/3559#issuecomment-1247586713
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=3559)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3559&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3559&resolved=false&types=BUG) [1 Bug](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3559&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3559&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3559&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3559&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3559&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3559&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3559&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3559&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3559&resolved=false&types=CODE_SMELL) [9 Code Smells](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3559&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=3559&metric=coverage&view=list) No Coverage information
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=3559&metric=duplicated_lines_density&view=list) No Duplication information
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ merged pull request #3559: HIVE-26496,Improvement to fetch operator to scan only delete_delta fo…
Posted by GitBox <gi...@apache.org>.
deniskuzZ merged PR #3559:
URL: https://github.com/apache/hive/pull/3559
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] sonarcloud[bot] commented on pull request #3559: HIVE-26496,Improvement to fetch operator to scan only delete_delta fo…
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #3559:
URL: https://github.com/apache/hive/pull/3559#issuecomment-1248307962
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=3559)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3559&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3559&resolved=false&types=BUG) [1 Bug](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3559&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3559&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3559&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3559&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3559&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3559&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3559&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3559&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3559&resolved=false&types=CODE_SMELL) [5 Code Smells](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3559&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=3559&metric=coverage&view=list) No Coverage information
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=3559&metric=duplicated_lines_density&view=list) No Duplication information
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] difin commented on a diff in pull request #3559: HIVE-26496,Improvement to fetch operator to scan only delete_delta fo…
Posted by GitBox <gi...@apache.org>.
difin commented on code in PR #3559:
URL: https://github.com/apache/hive/pull/3559#discussion_r962084263
##########
ql/src/java/org/apache/hadoop/hive/ql/io/orc/VectorizedOrcAcidRowBatchReader.java:
##########
@@ -885,7 +886,20 @@ static Path[] getDeleteDeltaDirsFromSplit(OrcSplit orcSplit,
} else {
throw new IllegalStateException("Split w/o base w/Acid 2.0??: " + path);
}
- return AcidUtils.deserializeDeleteDeltas(root, orcSplit.getDeltas(), pathToDeltaMetaData);
+
+ // filtering out delete_delta folders containing transactions that happened only before
+ // transactions of the split
+ List<AcidInputFormat.DeltaMetaData> deltas = orcSplit.getDeltas();
Review Comment:
Yes, I agree it is better and I made the filtering of irrelevant deltas to happen on the split initialization as suggested.
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] zabetak commented on a diff in pull request #3559: HIVE-26496,Improvement to fetch operator to scan only delete_delta fo…
Posted by GitBox <gi...@apache.org>.
zabetak commented on code in PR #3559:
URL: https://github.com/apache/hive/pull/3559#discussion_r973212714
##########
ql/src/java/org/apache/hadoop/hive/ql/io/orc/OrcSplit.java:
##########
@@ -104,7 +103,15 @@ public OrcSplit(Path path, Object fileId, long offset, long length, String[] hos
this.isOriginal = isOriginal;
this.hasBase = hasBase;
this.rootDir = rootDir;
- this.deltas.addAll(filterDeltasByBucketId(deltas, AcidUtils.parseBucketId(path)));
+ int bucketId = AcidUtils.parseBucketId(path);
Review Comment:
@difin @deniskuzZ
The local variable `bucketId` hides the instance variable; probably is was intentional but it is quite confusing; especially for someone that's gonna debug this code in the future. A comment/explanation would definitely help.
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] difin commented on a diff in pull request #3559: HIVE-26496,Improvement to fetch operator to scan only delete_delta fo…
Posted by GitBox <gi...@apache.org>.
difin commented on code in PR #3559:
URL: https://github.com/apache/hive/pull/3559#discussion_r971208288
##########
ql/src/java/org/apache/hadoop/hive/ql/io/orc/OrcSplit.java:
##########
@@ -104,13 +104,32 @@ public OrcSplit(Path path, Object fileId, long offset, long length, String[] hos
this.isOriginal = isOriginal;
this.hasBase = hasBase;
this.rootDir = rootDir;
- this.deltas.addAll(filterDeltasByBucketId(deltas, AcidUtils.parseBucketId(path)));
+ this.deltas.addAll(filterDeleteDeltasByWriteIds
+ (filterDeltasByBucketId(deltas, AcidUtils.parseBucketId(path)), conf));
this.projColsUncompressedSize = projectedDataSize <= 0 ? length : projectedDataSize;
// setting file length to Long.MAX_VALUE will let orc reader read file length from file system
this.fileLen = fileLen <= 0 ? Long.MAX_VALUE : fileLen;
this.syntheticAcidProps = syntheticAcidProps;
}
+ /**
+ * For every split we want to filter out the delete deltas that contain events that happened only
+ * in the past relative to the split
+ * @param deltas
+ * @param conf
+ * @return
+ */
+ protected List<AcidInputFormat.DeltaMetaData> filterDeleteDeltasByWriteIds(
+ List<AcidInputFormat.DeltaMetaData> deltas, Configuration conf) throws IOException {
+
+ AcidOutputFormat.Options orcSplitMinMaxWriteIds =
+ AcidUtils.parseBaseOrDeltaBucketFilename(getPath(), conf);
Review Comment:
Hi @deniskuzZ,
Many tests failed with the change of using AcidUtils.ParsedDeltaLight.parse() instead of AcidUtils.parseBaseOrDeltaBucketFilename() with exception on this line:
https://github.com/apache/hive/blob/e352684d5c87df1483444afc4c3ee897270bd413/ql/src/java/org/apache/hadoop/hive/ql/io/orc/OrcSplit.java#L108
As I understand the split is not always a delta folder, it can be some older format not supported by ParsedDeltaLight. I saw that ParsedDeltaLight.parse() is used in some cases internally in AcidUtils.parseBaseOrDeltaBucketFilename(), but not always. Can you please advise if I should revert to using AcidUtils.parseBaseOrDeltaBucketFilename() that worked in all cases or there is some better way?
https://github.com/apache/hive/blob/f6bd0eb80767adfa9ce9f47a6d02a4940903effb/ql/src/java/org/apache/hadoop/hive/ql/io/AcidUtils.java#L538-L552
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3559: HIVE-26496,Improvement to fetch operator to scan only delete_delta fo…
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3559:
URL: https://github.com/apache/hive/pull/3559#discussion_r959380862
##########
ql/src/java/org/apache/hadoop/hive/ql/io/orc/VectorizedOrcAcidRowBatchReader.java:
##########
@@ -885,7 +886,20 @@ static Path[] getDeleteDeltaDirsFromSplit(OrcSplit orcSplit,
} else {
throw new IllegalStateException("Split w/o base w/Acid 2.0??: " + path);
}
- return AcidUtils.deserializeDeleteDeltas(root, orcSplit.getDeltas(), pathToDeltaMetaData);
+
+ // filtering out delete_delta folders containing transactions that happened only before
+ // transactions of the split
+ List<AcidInputFormat.DeltaMetaData> deltas = orcSplit.getDeltas();
Review Comment:
wouldn't it be better if orcSplit.getDeltas() would already return only the relevant delete deltas? could we do the filtering on the split initialization?
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] difin commented on a diff in pull request #3559: HIVE-26496,Improvement to fetch operator to scan only delete_delta fo…
Posted by GitBox <gi...@apache.org>.
difin commented on code in PR #3559:
URL: https://github.com/apache/hive/pull/3559#discussion_r971190432
##########
ql/src/java/org/apache/hadoop/hive/ql/io/orc/OrcSplit.java:
##########
@@ -104,28 +104,43 @@ public OrcSplit(Path path, Object fileId, long offset, long length, String[] hos
this.isOriginal = isOriginal;
this.hasBase = hasBase;
this.rootDir = rootDir;
- this.deltas.addAll(filterDeltasByBucketId(deltas, AcidUtils.parseBucketId(path)));
+ int bucketId = AcidUtils.parseBucketId(path);
+ AcidUtils.ParsedDeltaLight parentDelta = AcidUtils.ParsedDeltaLight.parse(getPath().getParent());
Review Comment:
Hi @deniskuzZ,
Many tests failed with the change of using AcidUtils.ParsedDeltaLight.parse() instead of AcidUtils.parseBaseOrDeltaBucketFilename(). As I understand the split is not always a delta folder, it can be some older format not supported by ParsedDeltaLight. I saw that ParsedDeltaLight.parse() is used in some cases internally in AcidUtils.parseBaseOrDeltaBucketFilename(), but not always. Can you please advise if I should revert to using AcidUtils.parseBaseOrDeltaBucketFilename() that worked in all cases or there is some better way?
https://github.com/apache/hive/blob/master/ql/src/java/org/apache/hadoop/hive/ql/io/AcidUtils.java#539
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] difin commented on a diff in pull request #3559: HIVE-26496,Improvement to fetch operator to scan only delete_delta fo…
Posted by GitBox <gi...@apache.org>.
difin commented on code in PR #3559:
URL: https://github.com/apache/hive/pull/3559#discussion_r970927575
##########
ql/src/test/org/apache/hadoop/hive/ql/io/orc/TestOrcSplit.java:
##########
@@ -0,0 +1,181 @@
+/*
+ * 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.hadoop.hive.ql.io.orc;
+
+import org.apache.hadoop.fs.FileSystem;
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.hive.common.ValidReadTxnList;
+import org.apache.hadoop.hive.common.ValidTxnList;
+import org.apache.hadoop.hive.common.ValidWriteIdList;
+import org.apache.hadoop.hive.conf.HiveConf;
+import org.apache.hadoop.hive.metastore.api.hive_metastoreConstants;
+import org.apache.hadoop.hive.ql.io.*;
+import org.apache.hadoop.hive.ql.io.AcidUtils.Directory;
+import org.apache.hadoop.hive.serde2.objectinspector.ObjectInspector;
+import org.apache.hadoop.hive.serde2.objectinspector.ObjectInspectorFactory;
+import org.apache.hadoop.io.LongWritable;
+import org.apache.hadoop.mapred.JobConf;
+import org.apache.hadoop.mapred.Reporter;
+import org.apache.orc.OrcConf;
+import org.junit.Before;
+import org.junit.Test;
+
+import java.io.File;
+import java.util.*;
Review Comment:
replaced wildcard imports with concrete classes.
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3559: HIVE-26496,Improvement to fetch operator to scan only delete_delta fo…
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3559:
URL: https://github.com/apache/hive/pull/3559#discussion_r971635989
##########
ql/src/java/org/apache/hadoop/hive/ql/io/orc/OrcSplit.java:
##########
@@ -104,28 +103,44 @@ public OrcSplit(Path path, Object fileId, long offset, long length, String[] hos
this.isOriginal = isOriginal;
this.hasBase = hasBase;
this.rootDir = rootDir;
- this.deltas.addAll(filterDeltasByBucketId(deltas, AcidUtils.parseBucketId(path)));
+ int bucketId = AcidUtils.parseBucketId(path);
+ long minWriteId = !deltas.isEmpty() ?
+ AcidUtils.parseBaseOrDeltaBucketFilename(path, null).getMinimumWriteId() : -1;
+ this.deltas.addAll(
+ deltas.stream()
+ .filter(delta -> isQualifiedDeleteDeltasByWriteIds(delta, minWriteId))
Review Comment:
maybe simply
````
.filter(delta -> delta.getMaxWriteId() >= minWriteId)
````
for better readability, no need to go inside the method to check 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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] difin commented on a diff in pull request #3559: HIVE-26496,Improvement to fetch operator to scan only delete_delta fo…
Posted by GitBox <gi...@apache.org>.
difin commented on code in PR #3559:
URL: https://github.com/apache/hive/pull/3559#discussion_r971208288
##########
ql/src/java/org/apache/hadoop/hive/ql/io/orc/OrcSplit.java:
##########
@@ -104,13 +104,32 @@ public OrcSplit(Path path, Object fileId, long offset, long length, String[] hos
this.isOriginal = isOriginal;
this.hasBase = hasBase;
this.rootDir = rootDir;
- this.deltas.addAll(filterDeltasByBucketId(deltas, AcidUtils.parseBucketId(path)));
+ this.deltas.addAll(filterDeleteDeltasByWriteIds
+ (filterDeltasByBucketId(deltas, AcidUtils.parseBucketId(path)), conf));
this.projColsUncompressedSize = projectedDataSize <= 0 ? length : projectedDataSize;
// setting file length to Long.MAX_VALUE will let orc reader read file length from file system
this.fileLen = fileLen <= 0 ? Long.MAX_VALUE : fileLen;
this.syntheticAcidProps = syntheticAcidProps;
}
+ /**
+ * For every split we want to filter out the delete deltas that contain events that happened only
+ * in the past relative to the split
+ * @param deltas
+ * @param conf
+ * @return
+ */
+ protected List<AcidInputFormat.DeltaMetaData> filterDeleteDeltasByWriteIds(
+ List<AcidInputFormat.DeltaMetaData> deltas, Configuration conf) throws IOException {
+
+ AcidOutputFormat.Options orcSplitMinMaxWriteIds =
+ AcidUtils.parseBaseOrDeltaBucketFilename(getPath(), conf);
Review Comment:
Hi @deniskuzZ,
Many tests failed with the change of using AcidUtils.ParsedDeltaLight.parse() instead of AcidUtils.parseBaseOrDeltaBucketFilename() with exception on this line:
https://github.com/apache/hive/blob/e352684d5c87df1483444afc4c3ee897270bd413/ql/src/java/org/apache/hadoop/hive/ql/io/orc/OrcSplit.java#L108.
As I understand the split is not always a delta folder, it can be some older format not supported by ParsedDeltaLight. I saw that ParsedDeltaLight.parse() is used in some cases internally in AcidUtils.parseBaseOrDeltaBucketFilename(), but not always. Can you please advise if I should revert to using AcidUtils.parseBaseOrDeltaBucketFilename() that worked in all cases or there is some better way?
https://github.com/apache/hive/blob/f6bd0eb80767adfa9ce9f47a6d02a4940903effb/ql/src/java/org/apache/hadoop/hive/ql/io/AcidUtils.java#L538-L552
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] difin commented on pull request #3559: HIVE-26496,Improvement to fetch operator to scan only delete_delta fo…
Posted by GitBox <gi...@apache.org>.
difin commented on PR #3559:
URL: https://github.com/apache/hive/pull/3559#issuecomment-1245866883
Hi @abstractdog,
I added a unit test for OrcSplit which tests filtering of delete delta folders.
[TestOrcSplit](https://github.com/apache/hive/pull/3559/files#diff-8c565966e342b74baa2f0548bbed1a1a403e2f994041ecf87ec7893a210ae34b)
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3559: HIVE-26496,Improvement to fetch operator to scan only delete_delta fo…
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3559:
URL: https://github.com/apache/hive/pull/3559#discussion_r970696749
##########
ql/src/java/org/apache/hadoop/hive/ql/io/orc/OrcSplit.java:
##########
@@ -104,13 +104,32 @@ public OrcSplit(Path path, Object fileId, long offset, long length, String[] hos
this.isOriginal = isOriginal;
this.hasBase = hasBase;
this.rootDir = rootDir;
- this.deltas.addAll(filterDeltasByBucketId(deltas, AcidUtils.parseBucketId(path)));
+ this.deltas.addAll(filterDeleteDeltasByWriteIds
Review Comment:
could we transform this construct into a stream pipeline:
````
this.deltas = deltas.stream()
.filter(delta -> filterDeltasByBucketId(delta, AcidUtils.parseBucketId(path)))
.filter(delta -> filterDeleteDeltasByWriteIds(delta, conf))
.collect(Collectors.toList());
````
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] abstractdog commented on a diff in pull request #3559: HIVE-26496,Improvement to fetch operator to scan only delete_delta fo…
Posted by GitBox <gi...@apache.org>.
abstractdog commented on code in PR #3559:
URL: https://github.com/apache/hive/pull/3559#discussion_r970592299
##########
ql/src/test/org/apache/hadoop/hive/ql/io/orc/TestOrcSplit.java:
##########
@@ -0,0 +1,181 @@
+/*
+ * 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.hadoop.hive.ql.io.orc;
+
+import org.apache.hadoop.fs.FileSystem;
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.hive.common.ValidReadTxnList;
+import org.apache.hadoop.hive.common.ValidTxnList;
+import org.apache.hadoop.hive.common.ValidWriteIdList;
+import org.apache.hadoop.hive.conf.HiveConf;
+import org.apache.hadoop.hive.metastore.api.hive_metastoreConstants;
+import org.apache.hadoop.hive.ql.io.*;
+import org.apache.hadoop.hive.ql.io.AcidUtils.Directory;
+import org.apache.hadoop.hive.serde2.objectinspector.ObjectInspector;
+import org.apache.hadoop.hive.serde2.objectinspector.ObjectInspectorFactory;
+import org.apache.hadoop.io.LongWritable;
+import org.apache.hadoop.mapred.JobConf;
+import org.apache.hadoop.mapred.Reporter;
+import org.apache.orc.OrcConf;
+import org.junit.Before;
+import org.junit.Test;
+
+import java.io.File;
+import java.util.*;
+
+import static org.junit.Assert.*;
+
+/**
+ * Tests for OrcSplit class
+ */
+public class TestOrcSplit {
+
+ private JobConf conf;
+ private FileSystem fs;
+ private Path root;
+ private ObjectInspector inspector;
+ public static class DummyRow {
+ LongWritable field;
+ RecordIdentifier ROW__ID;
+
+ DummyRow(long val, long rowId, long origTxn, int bucket) {
+ field = new LongWritable(val);
+ bucket = BucketCodec.V1.encode(new AcidOutputFormat.Options(null).bucket(bucket));
+ ROW__ID = new RecordIdentifier(origTxn, bucket, rowId);
+ }
+
+ static String getColumnNamesProperty() {
+ return "field";
+ }
+ static String getColumnTypesProperty() {
+ return "bigint";
+ }
+
+ }
+
+ @Before
+ public void setup() throws Exception {
+ conf = new JobConf();
+ conf.set(hive_metastoreConstants.TABLE_IS_TRANSACTIONAL, "true");
+ conf.setBoolean(HiveConf.ConfVars.HIVE_TRANSACTIONAL_TABLE_SCAN.varname, true);
+ conf.set(hive_metastoreConstants.TABLE_TRANSACTIONAL_PROPERTIES, "default");
+ conf.setInt(HiveConf.ConfVars.HIVE_TXN_OPERATIONAL_PROPERTIES.varname,
+ AcidUtils.AcidOperationalProperties.getDefault().toInt());
+ conf.set(IOConstants.SCHEMA_EVOLUTION_COLUMNS, DummyRow.getColumnNamesProperty());
+ conf.set(IOConstants.SCHEMA_EVOLUTION_COLUMNS_TYPES, DummyRow.getColumnTypesProperty());
+ conf.setBoolean(HiveConf.ConfVars.HIVE_VECTORIZATION_ENABLED.varname, true);
+ conf.set(HiveConf.ConfVars.HIVE_ORC_SPLIT_STRATEGY.varname, "BI");
+ OrcConf.ROWS_BETWEEN_CHECKS.setLong(conf, 1);
+
+ Path workDir = new Path(System.getProperty("test.tmp.dir",
+ "target" + File.separator + "test" + File.separator + "tmp"));
+ root = new Path(workDir, "TestOrcSplit.testDump");
+ fs = root.getFileSystem(conf);
+ root = fs.makeQualified(root);
+ fs.delete(root, true);
+ synchronized (TestOrcFile.class) {
+ inspector = ObjectInspectorFactory.getReflectionObjectInspector
+ (DummyRow.class, ObjectInspectorFactory.ObjectInspectorOptions.JAVA);
+ }
+ }
+
+ private List<OrcInputFormat.SplitStrategy<?>> getSplitStrategies() throws Exception {
+ conf.setInt(HiveConf.ConfVars.HIVE_TXN_OPERATIONAL_PROPERTIES.varname,
+ AcidUtils.AcidOperationalProperties.getDefault().toInt());
+ OrcInputFormat.Context context = new OrcInputFormat.Context(conf);
+ OrcInputFormat.FileGenerator gen = new OrcInputFormat.FileGenerator(
+ context, () -> fs, root, false, null);
+ Directory adi = gen.call();
+ return OrcInputFormat.determineSplitStrategies(
+ null, context, adi.getFs(), adi.getPath(), adi.getFiles(), adi.getDeleteDeltas(),
+ null, null, true);
+ }
+
+ /**
+ * This test checks that a split filters out delete_delta folders which only have transactions that happened
+ * in the past relative to the current split
+ */
+ @Test
+ public void testDeleteDeltasFiltering() throws Exception {
+
+ int bucket = 0;
+ AcidOutputFormat.Options options = new AcidOutputFormat.Options(conf)
+ .filesystem(fs)
+ .bucket(bucket)
+ .writingBase(false)
+ .minimumWriteId(1)
+ .maximumWriteId(1)
+ .inspector(inspector)
+ .reporter(Reporter.NULL)
+ .recordIdColumn(1)
+ .finalDestination(root);
+
+ RecordUpdater updater = new OrcRecordUpdater(root, options);
+
+ // Inserting a new record and then deleting it, 3 times.
+ // Every insertion/deletion is in a separate transaction.
+ // When reading, whis will generate 3 splits, one per insert event.
+ for (int i=1; i<=5; i=i+2){
Review Comment:
nit: this is formatted properly as
```
for (int i = 1; i <= 5; i = i + 2) {
```
if you use eclipse, this one can be used:
https://github.com/apache/hive/blob/master/dev-support/eclipse-styles.xml
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] difin commented on a diff in pull request #3559: HIVE-26496,Improvement to fetch operator to scan only delete_delta fo…
Posted by GitBox <gi...@apache.org>.
difin commented on code in PR #3559:
URL: https://github.com/apache/hive/pull/3559#discussion_r971190432
##########
ql/src/java/org/apache/hadoop/hive/ql/io/orc/OrcSplit.java:
##########
@@ -104,28 +104,43 @@ public OrcSplit(Path path, Object fileId, long offset, long length, String[] hos
this.isOriginal = isOriginal;
this.hasBase = hasBase;
this.rootDir = rootDir;
- this.deltas.addAll(filterDeltasByBucketId(deltas, AcidUtils.parseBucketId(path)));
+ int bucketId = AcidUtils.parseBucketId(path);
+ AcidUtils.ParsedDeltaLight parentDelta = AcidUtils.ParsedDeltaLight.parse(getPath().getParent());
Review Comment:
Hi @deniskuzZ,
Many tests failed with the change of using AcidUtils.ParsedDeltaLight.parse() instead of AcidUtils.parseBaseOrDeltaBucketFilename(). As I understand the split is not always a delta folder, it can be some older format not supported by ParsedDeltaLight. I saw that ParsedDeltaLight.parse() is used in some cases internally in AcidUtils.parseBaseOrDeltaBucketFilename(), but not always. Can you please advise if I should revert to using AcidUtils.parseBaseOrDeltaBucketFilename() that worked in all cases or there is some better way?
https://github.com/apache/hive/blob/f6bd0eb80767adfa9ce9f47a6d02a4940903effb/ql/src/java/org/apache/hadoop/hive/ql/io/AcidUtils.java#L538-L552
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] difin commented on a diff in pull request #3559: HIVE-26496,Improvement to fetch operator to scan only delete_delta fo…
Posted by GitBox <gi...@apache.org>.
difin commented on code in PR #3559:
URL: https://github.com/apache/hive/pull/3559#discussion_r971221139
##########
ql/src/java/org/apache/hadoop/hive/ql/io/orc/OrcSplit.java:
##########
@@ -104,13 +104,32 @@ public OrcSplit(Path path, Object fileId, long offset, long length, String[] hos
this.isOriginal = isOriginal;
this.hasBase = hasBase;
this.rootDir = rootDir;
- this.deltas.addAll(filterDeltasByBucketId(deltas, AcidUtils.parseBucketId(path)));
+ this.deltas.addAll(filterDeleteDeltasByWriteIds
Review Comment:
deltas class member is marked as final and initialized before, that's why I couldn't reassign it and used addAll().
https://github.com/apache/hive/blob/e352684d5c87df1483444afc4c3ee897270bd413/ql/src/java/org/apache/hadoop/hive/ql/io/orc/OrcSplit.java#L67
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] abstractdog commented on a diff in pull request #3559: HIVE-26496,Improvement to fetch operator to scan only delete_delta fo…
Posted by GitBox <gi...@apache.org>.
abstractdog commented on code in PR #3559:
URL: https://github.com/apache/hive/pull/3559#discussion_r970589035
##########
ql/src/test/org/apache/hadoop/hive/ql/io/orc/TestOrcSplit.java:
##########
@@ -0,0 +1,181 @@
+/*
+ * 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.hadoop.hive.ql.io.orc;
+
+import org.apache.hadoop.fs.FileSystem;
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.hive.common.ValidReadTxnList;
+import org.apache.hadoop.hive.common.ValidTxnList;
+import org.apache.hadoop.hive.common.ValidWriteIdList;
+import org.apache.hadoop.hive.conf.HiveConf;
+import org.apache.hadoop.hive.metastore.api.hive_metastoreConstants;
+import org.apache.hadoop.hive.ql.io.*;
+import org.apache.hadoop.hive.ql.io.AcidUtils.Directory;
+import org.apache.hadoop.hive.serde2.objectinspector.ObjectInspector;
+import org.apache.hadoop.hive.serde2.objectinspector.ObjectInspectorFactory;
+import org.apache.hadoop.io.LongWritable;
+import org.apache.hadoop.mapred.JobConf;
+import org.apache.hadoop.mapred.Reporter;
+import org.apache.orc.OrcConf;
+import org.junit.Before;
+import org.junit.Test;
+
+import java.io.File;
+import java.util.*;
Review Comment:
we don't import with wildcards in general
honestly, I don't have a strong opinion about this :) but it tends to be avoided according to code reviews
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] abstractdog commented on a diff in pull request #3559: HIVE-26496,Improvement to fetch operator to scan only delete_delta fo…
Posted by GitBox <gi...@apache.org>.
abstractdog commented on code in PR #3559:
URL: https://github.com/apache/hive/pull/3559#discussion_r970592700
##########
ql/src/test/org/apache/hadoop/hive/ql/io/orc/TestOrcSplit.java:
##########
@@ -0,0 +1,181 @@
+/*
+ * 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.hadoop.hive.ql.io.orc;
+
+import org.apache.hadoop.fs.FileSystem;
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.hive.common.ValidReadTxnList;
+import org.apache.hadoop.hive.common.ValidTxnList;
+import org.apache.hadoop.hive.common.ValidWriteIdList;
+import org.apache.hadoop.hive.conf.HiveConf;
+import org.apache.hadoop.hive.metastore.api.hive_metastoreConstants;
+import org.apache.hadoop.hive.ql.io.*;
+import org.apache.hadoop.hive.ql.io.AcidUtils.Directory;
+import org.apache.hadoop.hive.serde2.objectinspector.ObjectInspector;
+import org.apache.hadoop.hive.serde2.objectinspector.ObjectInspectorFactory;
+import org.apache.hadoop.io.LongWritable;
+import org.apache.hadoop.mapred.JobConf;
+import org.apache.hadoop.mapred.Reporter;
+import org.apache.orc.OrcConf;
+import org.junit.Before;
+import org.junit.Test;
+
+import java.io.File;
+import java.util.*;
+
+import static org.junit.Assert.*;
+
+/**
+ * Tests for OrcSplit class
+ */
+public class TestOrcSplit {
+
+ private JobConf conf;
+ private FileSystem fs;
+ private Path root;
+ private ObjectInspector inspector;
+ public static class DummyRow {
+ LongWritable field;
+ RecordIdentifier ROW__ID;
+
+ DummyRow(long val, long rowId, long origTxn, int bucket) {
+ field = new LongWritable(val);
+ bucket = BucketCodec.V1.encode(new AcidOutputFormat.Options(null).bucket(bucket));
+ ROW__ID = new RecordIdentifier(origTxn, bucket, rowId);
+ }
+
+ static String getColumnNamesProperty() {
+ return "field";
+ }
+ static String getColumnTypesProperty() {
+ return "bigint";
+ }
+
+ }
+
+ @Before
+ public void setup() throws Exception {
+ conf = new JobConf();
+ conf.set(hive_metastoreConstants.TABLE_IS_TRANSACTIONAL, "true");
+ conf.setBoolean(HiveConf.ConfVars.HIVE_TRANSACTIONAL_TABLE_SCAN.varname, true);
+ conf.set(hive_metastoreConstants.TABLE_TRANSACTIONAL_PROPERTIES, "default");
+ conf.setInt(HiveConf.ConfVars.HIVE_TXN_OPERATIONAL_PROPERTIES.varname,
+ AcidUtils.AcidOperationalProperties.getDefault().toInt());
+ conf.set(IOConstants.SCHEMA_EVOLUTION_COLUMNS, DummyRow.getColumnNamesProperty());
+ conf.set(IOConstants.SCHEMA_EVOLUTION_COLUMNS_TYPES, DummyRow.getColumnTypesProperty());
+ conf.setBoolean(HiveConf.ConfVars.HIVE_VECTORIZATION_ENABLED.varname, true);
+ conf.set(HiveConf.ConfVars.HIVE_ORC_SPLIT_STRATEGY.varname, "BI");
+ OrcConf.ROWS_BETWEEN_CHECKS.setLong(conf, 1);
+
+ Path workDir = new Path(System.getProperty("test.tmp.dir",
+ "target" + File.separator + "test" + File.separator + "tmp"));
+ root = new Path(workDir, "TestOrcSplit.testDump");
+ fs = root.getFileSystem(conf);
+ root = fs.makeQualified(root);
+ fs.delete(root, true);
+ synchronized (TestOrcFile.class) {
+ inspector = ObjectInspectorFactory.getReflectionObjectInspector
+ (DummyRow.class, ObjectInspectorFactory.ObjectInspectorOptions.JAVA);
+ }
+ }
+
+ private List<OrcInputFormat.SplitStrategy<?>> getSplitStrategies() throws Exception {
+ conf.setInt(HiveConf.ConfVars.HIVE_TXN_OPERATIONAL_PROPERTIES.varname,
+ AcidUtils.AcidOperationalProperties.getDefault().toInt());
+ OrcInputFormat.Context context = new OrcInputFormat.Context(conf);
+ OrcInputFormat.FileGenerator gen = new OrcInputFormat.FileGenerator(
+ context, () -> fs, root, false, null);
+ Directory adi = gen.call();
+ return OrcInputFormat.determineSplitStrategies(
+ null, context, adi.getFs(), adi.getPath(), adi.getFiles(), adi.getDeleteDeltas(),
+ null, null, true);
+ }
+
+ /**
+ * This test checks that a split filters out delete_delta folders which only have transactions that happened
+ * in the past relative to the current split
+ */
+ @Test
+ public void testDeleteDeltasFiltering() throws Exception {
+
+ int bucket = 0;
+ AcidOutputFormat.Options options = new AcidOutputFormat.Options(conf)
+ .filesystem(fs)
+ .bucket(bucket)
+ .writingBase(false)
+ .minimumWriteId(1)
+ .maximumWriteId(1)
+ .inspector(inspector)
+ .reporter(Reporter.NULL)
+ .recordIdColumn(1)
+ .finalDestination(root);
+
+ RecordUpdater updater = new OrcRecordUpdater(root, options);
+
+ // Inserting a new record and then deleting it, 3 times.
+ // Every insertion/deletion is in a separate transaction.
+ // When reading, whis will generate 3 splits, one per insert event.
Review Comment:
"this will"
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] difin commented on a diff in pull request #3559: HIVE-26496,Improvement to fetch operator to scan only delete_delta fo…
Posted by GitBox <gi...@apache.org>.
difin commented on code in PR #3559:
URL: https://github.com/apache/hive/pull/3559#discussion_r971208288
##########
ql/src/java/org/apache/hadoop/hive/ql/io/orc/OrcSplit.java:
##########
@@ -104,13 +104,32 @@ public OrcSplit(Path path, Object fileId, long offset, long length, String[] hos
this.isOriginal = isOriginal;
this.hasBase = hasBase;
this.rootDir = rootDir;
- this.deltas.addAll(filterDeltasByBucketId(deltas, AcidUtils.parseBucketId(path)));
+ this.deltas.addAll(filterDeleteDeltasByWriteIds
+ (filterDeltasByBucketId(deltas, AcidUtils.parseBucketId(path)), conf));
this.projColsUncompressedSize = projectedDataSize <= 0 ? length : projectedDataSize;
// setting file length to Long.MAX_VALUE will let orc reader read file length from file system
this.fileLen = fileLen <= 0 ? Long.MAX_VALUE : fileLen;
this.syntheticAcidProps = syntheticAcidProps;
}
+ /**
+ * For every split we want to filter out the delete deltas that contain events that happened only
+ * in the past relative to the split
+ * @param deltas
+ * @param conf
+ * @return
+ */
+ protected List<AcidInputFormat.DeltaMetaData> filterDeleteDeltasByWriteIds(
+ List<AcidInputFormat.DeltaMetaData> deltas, Configuration conf) throws IOException {
+
+ AcidOutputFormat.Options orcSplitMinMaxWriteIds =
+ AcidUtils.parseBaseOrDeltaBucketFilename(getPath(), conf);
Review Comment:
Hi @deniskuzZ,
Many tests failed with the change of using AcidUtils.ParsedDeltaLight.parse() instead of AcidUtils.parseBaseOrDeltaBucketFilename(). As I understand the split is not always a delta folder, it can be some older format not supported by ParsedDeltaLight. I saw that ParsedDeltaLight.parse() is used in some cases internally in AcidUtils.parseBaseOrDeltaBucketFilename(), but not always. Can you please advise if I should revert to using AcidUtils.parseBaseOrDeltaBucketFilename() that worked in all cases or there is some better way?
https://github.com/apache/hive/blob/f6bd0eb80767adfa9ce9f47a6d02a4940903effb/ql/src/java/org/apache/hadoop/hive/ql/io/AcidUtils.java#L538-L552
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] difin commented on a diff in pull request #3559: HIVE-26496,Improvement to fetch operator to scan only delete_delta fo…
Posted by GitBox <gi...@apache.org>.
difin commented on code in PR #3559:
URL: https://github.com/apache/hive/pull/3559#discussion_r971221139
##########
ql/src/java/org/apache/hadoop/hive/ql/io/orc/OrcSplit.java:
##########
@@ -104,13 +104,32 @@ public OrcSplit(Path path, Object fileId, long offset, long length, String[] hos
this.isOriginal = isOriginal;
this.hasBase = hasBase;
this.rootDir = rootDir;
- this.deltas.addAll(filterDeltasByBucketId(deltas, AcidUtils.parseBucketId(path)));
+ this.deltas.addAll(filterDeleteDeltasByWriteIds
Review Comment:
deltas class member is marked as final, that's why I couldn't reassign it and used addAll().
https://github.com/apache/hive/blob/e352684d5c87df1483444afc4c3ee897270bd413/ql/src/java/org/apache/hadoop/hive/ql/io/orc/OrcSplit.java#L67
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3559: HIVE-26496,Improvement to fetch operator to scan only delete_delta fo…
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3559:
URL: https://github.com/apache/hive/pull/3559#discussion_r971375969
##########
ql/src/java/org/apache/hadoop/hive/ql/io/orc/OrcSplit.java:
##########
@@ -104,13 +104,32 @@ public OrcSplit(Path path, Object fileId, long offset, long length, String[] hos
this.isOriginal = isOriginal;
this.hasBase = hasBase;
this.rootDir = rootDir;
- this.deltas.addAll(filterDeltasByBucketId(deltas, AcidUtils.parseBucketId(path)));
+ this.deltas.addAll(filterDeleteDeltasByWriteIds
+ (filterDeltasByBucketId(deltas, AcidUtils.parseBucketId(path)), conf));
this.projColsUncompressedSize = projectedDataSize <= 0 ? length : projectedDataSize;
// setting file length to Long.MAX_VALUE will let orc reader read file length from file system
this.fileLen = fileLen <= 0 ? Long.MAX_VALUE : fileLen;
this.syntheticAcidProps = syntheticAcidProps;
}
+ /**
+ * For every split we want to filter out the delete deltas that contain events that happened only
+ * in the past relative to the split
+ * @param deltas
+ * @param conf
+ * @return
+ */
+ protected List<AcidInputFormat.DeltaMetaData> filterDeleteDeltasByWriteIds(
+ List<AcidInputFormat.DeltaMetaData> deltas, Configuration conf) throws IOException {
+
+ AcidOutputFormat.Options orcSplitMinMaxWriteIds =
+ AcidUtils.parseBaseOrDeltaBucketFilename(getPath(), conf);
Review Comment:
root path could be anything: delta/delete-delta/base. the prev version was actually correct
````
long minWriteId = !deltas.isEmpty() ?
AcidUtils.parseBaseOrDeltaBucketFilename(path, null).getMinimumWriteId() : -1;
this.deltas.addAll(
deltas.stream()
.filter(delta -> isQualifiedDeleteDeltasByWriteIds(delta, minWriteId))
.flatMap(delta -> filterDeltasByBucketId(delta, bucketId))
.collect(Collectors.toList()));
````
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] difin commented on a diff in pull request #3559: HIVE-26496,Improvement to fetch operator to scan only delete_delta fo…
Posted by GitBox <gi...@apache.org>.
difin commented on code in PR #3559:
URL: https://github.com/apache/hive/pull/3559#discussion_r971868483
##########
ql/src/java/org/apache/hadoop/hive/ql/io/orc/OrcSplit.java:
##########
@@ -104,13 +104,32 @@ public OrcSplit(Path path, Object fileId, long offset, long length, String[] hos
this.isOriginal = isOriginal;
this.hasBase = hasBase;
this.rootDir = rootDir;
- this.deltas.addAll(filterDeltasByBucketId(deltas, AcidUtils.parseBucketId(path)));
+ this.deltas.addAll(filterDeleteDeltasByWriteIds
+ (filterDeltasByBucketId(deltas, AcidUtils.parseBucketId(path)), conf));
this.projColsUncompressedSize = projectedDataSize <= 0 ? length : projectedDataSize;
// setting file length to Long.MAX_VALUE will let orc reader read file length from file system
this.fileLen = fileLen <= 0 ? Long.MAX_VALUE : fileLen;
this.syntheticAcidProps = syntheticAcidProps;
}
+ /**
+ * For every split we want to filter out the delete deltas that contain events that happened only
+ * in the past relative to the split
+ * @param deltas
+ * @param conf
+ * @return
+ */
+ protected List<AcidInputFormat.DeltaMetaData> filterDeleteDeltasByWriteIds(
+ List<AcidInputFormat.DeltaMetaData> deltas, Configuration conf) throws IOException {
+
+ AcidOutputFormat.Options orcSplitMinMaxWriteIds =
+ AcidUtils.parseBaseOrDeltaBucketFilename(getPath(), conf);
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] sonarcloud[bot] commented on pull request #3559: HIVE-26496,Improvement to fetch operator to scan only delete_delta fo…
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #3559:
URL: https://github.com/apache/hive/pull/3559#issuecomment-1235814585
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=3559)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3559&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3559&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3559&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3559&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3559&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3559&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3559&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3559&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3559&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3559&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3559&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3559&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=3559&metric=coverage&view=list) No Coverage information
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=3559&metric=duplicated_lines_density&view=list) No Duplication information
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] difin commented on a diff in pull request #3559: HIVE-26496,Improvement to fetch operator to scan only delete_delta fo…
Posted by GitBox <gi...@apache.org>.
difin commented on code in PR #3559:
URL: https://github.com/apache/hive/pull/3559#discussion_r962084263
##########
ql/src/java/org/apache/hadoop/hive/ql/io/orc/VectorizedOrcAcidRowBatchReader.java:
##########
@@ -885,7 +886,20 @@ static Path[] getDeleteDeltaDirsFromSplit(OrcSplit orcSplit,
} else {
throw new IllegalStateException("Split w/o base w/Acid 2.0??: " + path);
}
- return AcidUtils.deserializeDeleteDeltas(root, orcSplit.getDeltas(), pathToDeltaMetaData);
+
+ // filtering out delete_delta folders containing transactions that happened only before
+ // transactions of the split
+ List<AcidInputFormat.DeltaMetaData> deltas = orcSplit.getDeltas();
Review Comment:
Yes, I agree it is better and I made the filtering or irrelevant deltas to happen on the split initialization as suggested.
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] sonarcloud[bot] commented on pull request #3559: HIVE-26496,Improvement to fetch operator to scan only delete_delta fo…
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #3559:
URL: https://github.com/apache/hive/pull/3559#issuecomment-1245873615
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=3559)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3559&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3559&resolved=false&types=BUG) [1 Bug](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3559&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3559&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3559&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3559&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3559&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3559&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3559&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3559&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3559&resolved=false&types=CODE_SMELL) [44 Code Smells](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3559&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=3559&metric=coverage&view=list) No Coverage information
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=3559&metric=duplicated_lines_density&view=list) No Duplication information
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] difin commented on a diff in pull request #3559: HIVE-26496,Improvement to fetch operator to scan only delete_delta fo…
Posted by GitBox <gi...@apache.org>.
difin commented on code in PR #3559:
URL: https://github.com/apache/hive/pull/3559#discussion_r971190432
##########
ql/src/java/org/apache/hadoop/hive/ql/io/orc/OrcSplit.java:
##########
@@ -104,28 +104,43 @@ public OrcSplit(Path path, Object fileId, long offset, long length, String[] hos
this.isOriginal = isOriginal;
this.hasBase = hasBase;
this.rootDir = rootDir;
- this.deltas.addAll(filterDeltasByBucketId(deltas, AcidUtils.parseBucketId(path)));
+ int bucketId = AcidUtils.parseBucketId(path);
+ AcidUtils.ParsedDeltaLight parentDelta = AcidUtils.ParsedDeltaLight.parse(getPath().getParent());
Review Comment:
Hi @deniskuzZ,
Many tests failed with the change of using AcidUtils.ParsedDeltaLight.parse() instead of AcidUtils.parseBaseOrDeltaBucketFilename(). As I understand the split is not always a delta folder, it can be some older format not supported by ParsedDeltaLight. I saw that ParsedDeltaLight.parse() is used in some cases internally in AcidUtils.parseBaseOrDeltaBucketFilename(), but not always. Can you please advise if I should revert to using AcidUtils.parseBaseOrDeltaBucketFilename() that worked in all cases or there is some better way?
https://github.com/apache/hive/blob/f6bd0eb80767adfa9ce9f47a6d02a4940903effb/ql/src/java/org/apache/hadoop/hive/ql/io/AcidUtils.java#L538-L552
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] difin commented on a diff in pull request #3559: HIVE-26496,Improvement to fetch operator to scan only delete_delta fo…
Posted by GitBox <gi...@apache.org>.
difin commented on code in PR #3559:
URL: https://github.com/apache/hive/pull/3559#discussion_r971221139
##########
ql/src/java/org/apache/hadoop/hive/ql/io/orc/OrcSplit.java:
##########
@@ -104,13 +104,32 @@ public OrcSplit(Path path, Object fileId, long offset, long length, String[] hos
this.isOriginal = isOriginal;
this.hasBase = hasBase;
this.rootDir = rootDir;
- this.deltas.addAll(filterDeltasByBucketId(deltas, AcidUtils.parseBucketId(path)));
+ this.deltas.addAll(filterDeleteDeltasByWriteIds
Review Comment:
deltas class member is marked as final, that's why I couldn't reassign it and used addAll().
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3559: HIVE-26496,Improvement to fetch operator to scan only delete_delta fo…
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3559:
URL: https://github.com/apache/hive/pull/3559#discussion_r971626356
##########
ql/src/java/org/apache/hadoop/hive/ql/io/orc/OrcSplit.java:
##########
@@ -94,7 +93,7 @@ protected OrcSplit() {
public OrcSplit(Path path, Object fileId, long offset, long length, String[] hosts,
OrcTail orcTail, boolean isOriginal, boolean hasBase,
List<AcidInputFormat.DeltaMetaData> deltas, long projectedDataSize, long fileLen, Path rootDir,
- OffsetAndBucketProperty syntheticAcidProps) {
+ OffsetAndBucketProperty syntheticAcidProps) throws IOException {
Review Comment:
please remove `throws exception` from the `parseBaseOrDeltaBucketFilename` declaration and here.
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3559: HIVE-26496,Improvement to fetch operator to scan only delete_delta fo…
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3559:
URL: https://github.com/apache/hive/pull/3559#discussion_r971375969
##########
ql/src/java/org/apache/hadoop/hive/ql/io/orc/OrcSplit.java:
##########
@@ -104,13 +104,32 @@ public OrcSplit(Path path, Object fileId, long offset, long length, String[] hos
this.isOriginal = isOriginal;
this.hasBase = hasBase;
this.rootDir = rootDir;
- this.deltas.addAll(filterDeltasByBucketId(deltas, AcidUtils.parseBucketId(path)));
+ this.deltas.addAll(filterDeleteDeltasByWriteIds
+ (filterDeltasByBucketId(deltas, AcidUtils.parseBucketId(path)), conf));
this.projColsUncompressedSize = projectedDataSize <= 0 ? length : projectedDataSize;
// setting file length to Long.MAX_VALUE will let orc reader read file length from file system
this.fileLen = fileLen <= 0 ? Long.MAX_VALUE : fileLen;
this.syntheticAcidProps = syntheticAcidProps;
}
+ /**
+ * For every split we want to filter out the delete deltas that contain events that happened only
+ * in the past relative to the split
+ * @param deltas
+ * @param conf
+ * @return
+ */
+ protected List<AcidInputFormat.DeltaMetaData> filterDeleteDeltasByWriteIds(
+ List<AcidInputFormat.DeltaMetaData> deltas, Configuration conf) throws IOException {
+
+ AcidOutputFormat.Options orcSplitMinMaxWriteIds =
+ AcidUtils.parseBaseOrDeltaBucketFilename(getPath(), conf);
Review Comment:
root path could be anything: delta/base. the prev version was actually correct
````
long minWriteId = !deltas.isEmpty() ?
AcidUtils.parseBaseOrDeltaBucketFilename(path, null).getMinimumWriteId() : -1;
this.deltas.addAll(
deltas.stream()
.filter(delta -> isQualifiedDeleteDeltasByWriteIds(delta, minWriteId))
.flatMap(delta -> filterDeltasByBucketId(delta, bucketId))
.collect(Collectors.toList()));
````
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] sonarcloud[bot] commented on pull request #3559: HIVE-26496,Improvement to fetch operator to scan only delete_delta fo…
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #3559:
URL: https://github.com/apache/hive/pull/3559#issuecomment-1248040368
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=3559)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3559&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3559&resolved=false&types=BUG) [1 Bug](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3559&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3559&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3559&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3559&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3559&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3559&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3559&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3559&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3559&resolved=false&types=CODE_SMELL) [6 Code Smells](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3559&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=3559&metric=coverage&view=list) No Coverage information
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=3559&metric=duplicated_lines_density&view=list) No Duplication information
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] zabetak commented on a diff in pull request #3559: HIVE-26496,Improvement to fetch operator to scan only delete_delta fo…
Posted by GitBox <gi...@apache.org>.
zabetak commented on code in PR #3559:
URL: https://github.com/apache/hive/pull/3559#discussion_r973212714
##########
ql/src/java/org/apache/hadoop/hive/ql/io/orc/OrcSplit.java:
##########
@@ -104,7 +103,15 @@ public OrcSplit(Path path, Object fileId, long offset, long length, String[] hos
this.isOriginal = isOriginal;
this.hasBase = hasBase;
this.rootDir = rootDir;
- this.deltas.addAll(filterDeltasByBucketId(deltas, AcidUtils.parseBucketId(path)));
+ int bucketId = AcidUtils.parseBucketId(path);
Review Comment:
@difin @deniskuzZ
The local variable hides the instance variable; probably is was intentional but it is quite confusing; especially for someone that's gonna debug this code in the future. A comment/explanation would definitely help.
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] difin commented on a diff in pull request #3559: HIVE-26496,Improvement to fetch operator to scan only delete_delta fo…
Posted by GitBox <gi...@apache.org>.
difin commented on code in PR #3559:
URL: https://github.com/apache/hive/pull/3559#discussion_r972026124
##########
ql/src/java/org/apache/hadoop/hive/ql/io/orc/OrcSplit.java:
##########
@@ -104,28 +103,44 @@ public OrcSplit(Path path, Object fileId, long offset, long length, String[] hos
this.isOriginal = isOriginal;
this.hasBase = hasBase;
this.rootDir = rootDir;
- this.deltas.addAll(filterDeltasByBucketId(deltas, AcidUtils.parseBucketId(path)));
+ int bucketId = AcidUtils.parseBucketId(path);
+ long minWriteId = !deltas.isEmpty() ?
+ AcidUtils.parseBaseOrDeltaBucketFilename(path, null).getMinimumWriteId() : -1;
+ this.deltas.addAll(
+ deltas.stream()
+ .filter(delta -> isQualifiedDeleteDeltasByWriteIds(delta, minWriteId))
Review Comment:
I was thinking it is better to have a method with a good self-describing name for more readability, but I agree that now that the method became very small it is better to do as you suggested. I did that and added a comment on top for explanation.
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] difin commented on a diff in pull request #3559: HIVE-26496,Improvement to fetch operator to scan only delete_delta fo…
Posted by GitBox <gi...@apache.org>.
difin commented on code in PR #3559:
URL: https://github.com/apache/hive/pull/3559#discussion_r974613822
##########
ql/src/java/org/apache/hadoop/hive/ql/io/orc/OrcSplit.java:
##########
@@ -104,7 +103,15 @@ public OrcSplit(Path path, Object fileId, long offset, long length, String[] hos
this.isOriginal = isOriginal;
this.hasBase = hasBase;
this.rootDir = rootDir;
- this.deltas.addAll(filterDeltasByBucketId(deltas, AcidUtils.parseBucketId(path)));
+ int bucketId = AcidUtils.parseBucketId(path);
Review Comment:
Hi @deniskuzZ @zabetak
Fixed in PR https://github.com/apache/hive/pull/3606
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3559: HIVE-26496,Improvement to fetch operator to scan only delete_delta fo…
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3559:
URL: https://github.com/apache/hive/pull/3559#discussion_r970701167
##########
ql/src/java/org/apache/hadoop/hive/ql/io/orc/OrcSplit.java:
##########
@@ -104,13 +104,32 @@ public OrcSplit(Path path, Object fileId, long offset, long length, String[] hos
this.isOriginal = isOriginal;
this.hasBase = hasBase;
this.rootDir = rootDir;
- this.deltas.addAll(filterDeltasByBucketId(deltas, AcidUtils.parseBucketId(path)));
+ this.deltas.addAll(filterDeleteDeltasByWriteIds
+ (filterDeltasByBucketId(deltas, AcidUtils.parseBucketId(path)), conf));
this.projColsUncompressedSize = projectedDataSize <= 0 ? length : projectedDataSize;
// setting file length to Long.MAX_VALUE will let orc reader read file length from file system
this.fileLen = fileLen <= 0 ? Long.MAX_VALUE : fileLen;
this.syntheticAcidProps = syntheticAcidProps;
}
+ /**
+ * For every split we want to filter out the delete deltas that contain events that happened only
+ * in the past relative to the split
+ * @param deltas
+ * @param conf
+ * @return
+ */
+ protected List<AcidInputFormat.DeltaMetaData> filterDeleteDeltasByWriteIds(
+ List<AcidInputFormat.DeltaMetaData> deltas, Configuration conf) throws IOException {
+
+ AcidOutputFormat.Options orcSplitMinMaxWriteIds =
+ AcidUtils.parseBaseOrDeltaBucketFilename(getPath(), conf);
Review Comment:
why not simply `ParsedDeltaLight.parse(bucketFile.getParent())`?
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] difin commented on a diff in pull request #3559: HIVE-26496,Improvement to fetch operator to scan only delete_delta fo…
Posted by GitBox <gi...@apache.org>.
difin commented on code in PR #3559:
URL: https://github.com/apache/hive/pull/3559#discussion_r970928073
##########
ql/src/test/org/apache/hadoop/hive/ql/io/orc/TestOrcSplit.java:
##########
@@ -0,0 +1,181 @@
+/*
+ * 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.hadoop.hive.ql.io.orc;
+
+import org.apache.hadoop.fs.FileSystem;
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.hive.common.ValidReadTxnList;
+import org.apache.hadoop.hive.common.ValidTxnList;
+import org.apache.hadoop.hive.common.ValidWriteIdList;
+import org.apache.hadoop.hive.conf.HiveConf;
+import org.apache.hadoop.hive.metastore.api.hive_metastoreConstants;
+import org.apache.hadoop.hive.ql.io.*;
+import org.apache.hadoop.hive.ql.io.AcidUtils.Directory;
+import org.apache.hadoop.hive.serde2.objectinspector.ObjectInspector;
+import org.apache.hadoop.hive.serde2.objectinspector.ObjectInspectorFactory;
+import org.apache.hadoop.io.LongWritable;
+import org.apache.hadoop.mapred.JobConf;
+import org.apache.hadoop.mapred.Reporter;
+import org.apache.orc.OrcConf;
+import org.junit.Before;
+import org.junit.Test;
+
+import java.io.File;
+import java.util.*;
+
+import static org.junit.Assert.*;
+
+/**
+ * Tests for OrcSplit class
+ */
+public class TestOrcSplit {
+
+ private JobConf conf;
+ private FileSystem fs;
+ private Path root;
+ private ObjectInspector inspector;
+ public static class DummyRow {
+ LongWritable field;
+ RecordIdentifier ROW__ID;
+
+ DummyRow(long val, long rowId, long origTxn, int bucket) {
+ field = new LongWritable(val);
+ bucket = BucketCodec.V1.encode(new AcidOutputFormat.Options(null).bucket(bucket));
+ ROW__ID = new RecordIdentifier(origTxn, bucket, rowId);
+ }
+
+ static String getColumnNamesProperty() {
+ return "field";
+ }
+ static String getColumnTypesProperty() {
+ return "bigint";
+ }
+
+ }
+
+ @Before
+ public void setup() throws Exception {
+ conf = new JobConf();
+ conf.set(hive_metastoreConstants.TABLE_IS_TRANSACTIONAL, "true");
+ conf.setBoolean(HiveConf.ConfVars.HIVE_TRANSACTIONAL_TABLE_SCAN.varname, true);
+ conf.set(hive_metastoreConstants.TABLE_TRANSACTIONAL_PROPERTIES, "default");
+ conf.setInt(HiveConf.ConfVars.HIVE_TXN_OPERATIONAL_PROPERTIES.varname,
+ AcidUtils.AcidOperationalProperties.getDefault().toInt());
+ conf.set(IOConstants.SCHEMA_EVOLUTION_COLUMNS, DummyRow.getColumnNamesProperty());
+ conf.set(IOConstants.SCHEMA_EVOLUTION_COLUMNS_TYPES, DummyRow.getColumnTypesProperty());
+ conf.setBoolean(HiveConf.ConfVars.HIVE_VECTORIZATION_ENABLED.varname, true);
+ conf.set(HiveConf.ConfVars.HIVE_ORC_SPLIT_STRATEGY.varname, "BI");
+ OrcConf.ROWS_BETWEEN_CHECKS.setLong(conf, 1);
+
+ Path workDir = new Path(System.getProperty("test.tmp.dir",
+ "target" + File.separator + "test" + File.separator + "tmp"));
+ root = new Path(workDir, "TestOrcSplit.testDump");
+ fs = root.getFileSystem(conf);
+ root = fs.makeQualified(root);
+ fs.delete(root, true);
+ synchronized (TestOrcFile.class) {
+ inspector = ObjectInspectorFactory.getReflectionObjectInspector
+ (DummyRow.class, ObjectInspectorFactory.ObjectInspectorOptions.JAVA);
+ }
+ }
+
+ private List<OrcInputFormat.SplitStrategy<?>> getSplitStrategies() throws Exception {
+ conf.setInt(HiveConf.ConfVars.HIVE_TXN_OPERATIONAL_PROPERTIES.varname,
+ AcidUtils.AcidOperationalProperties.getDefault().toInt());
+ OrcInputFormat.Context context = new OrcInputFormat.Context(conf);
+ OrcInputFormat.FileGenerator gen = new OrcInputFormat.FileGenerator(
+ context, () -> fs, root, false, null);
+ Directory adi = gen.call();
+ return OrcInputFormat.determineSplitStrategies(
+ null, context, adi.getFs(), adi.getPath(), adi.getFiles(), adi.getDeleteDeltas(),
+ null, null, true);
+ }
+
+ /**
+ * This test checks that a split filters out delete_delta folders which only have transactions that happened
+ * in the past relative to the current split
+ */
+ @Test
+ public void testDeleteDeltasFiltering() throws Exception {
+
+ int bucket = 0;
+ AcidOutputFormat.Options options = new AcidOutputFormat.Options(conf)
+ .filesystem(fs)
+ .bucket(bucket)
+ .writingBase(false)
+ .minimumWriteId(1)
+ .maximumWriteId(1)
+ .inspector(inspector)
+ .reporter(Reporter.NULL)
+ .recordIdColumn(1)
+ .finalDestination(root);
+
+ RecordUpdater updater = new OrcRecordUpdater(root, options);
+
+ // Inserting a new record and then deleting it, 3 times.
+ // Every insertion/deletion is in a separate transaction.
+ // When reading, whis will generate 3 splits, one per insert event.
+ for (int i=1; i<=5; i=i+2){
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] difin commented on a diff in pull request #3559: HIVE-26496,Improvement to fetch operator to scan only delete_delta fo…
Posted by GitBox <gi...@apache.org>.
difin commented on code in PR #3559:
URL: https://github.com/apache/hive/pull/3559#discussion_r970930245
##########
ql/src/java/org/apache/hadoop/hive/ql/io/orc/OrcSplit.java:
##########
@@ -104,13 +104,32 @@ public OrcSplit(Path path, Object fileId, long offset, long length, String[] hos
this.isOriginal = isOriginal;
this.hasBase = hasBase;
this.rootDir = rootDir;
- this.deltas.addAll(filterDeltasByBucketId(deltas, AcidUtils.parseBucketId(path)));
+ this.deltas.addAll(filterDeleteDeltasByWriteIds
Review Comment:
done, replaced nested function calls with stream processing.
##########
ql/src/java/org/apache/hadoop/hive/ql/io/orc/OrcSplit.java:
##########
@@ -104,13 +104,32 @@ public OrcSplit(Path path, Object fileId, long offset, long length, String[] hos
this.isOriginal = isOriginal;
this.hasBase = hasBase;
this.rootDir = rootDir;
- this.deltas.addAll(filterDeltasByBucketId(deltas, AcidUtils.parseBucketId(path)));
+ this.deltas.addAll(filterDeleteDeltasByWriteIds
+ (filterDeltasByBucketId(deltas, AcidUtils.parseBucketId(path)), conf));
this.projColsUncompressedSize = projectedDataSize <= 0 ? length : projectedDataSize;
// setting file length to Long.MAX_VALUE will let orc reader read file length from file system
this.fileLen = fileLen <= 0 ? Long.MAX_VALUE : fileLen;
this.syntheticAcidProps = syntheticAcidProps;
}
+ /**
+ * For every split we want to filter out the delete deltas that contain events that happened only
+ * in the past relative to the split
+ * @param deltas
+ * @param conf
+ * @return
+ */
+ protected List<AcidInputFormat.DeltaMetaData> filterDeleteDeltasByWriteIds(
+ List<AcidInputFormat.DeltaMetaData> deltas, Configuration conf) throws IOException {
+
+ AcidOutputFormat.Options orcSplitMinMaxWriteIds =
+ AcidUtils.parseBaseOrDeltaBucketFilename(getPath(), conf);
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] sonarcloud[bot] commented on pull request #3559: HIVE-26496,Improvement to fetch operator to scan only delete_delta fo…
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #3559:
URL: https://github.com/apache/hive/pull/3559#issuecomment-1247057159
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_hive&pullRequest=3559)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3559&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3559&resolved=false&types=BUG) [1 Bug](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3559&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3559&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3559&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3559&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3559&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3559&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive&pullRequest=3559&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3559&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3559&resolved=false&types=CODE_SMELL) [45 Code Smells](https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=3559&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=3559&metric=coverage&view=list) No Coverage information
[](https://sonarcloud.io/component_measures?id=apache_hive&pullRequest=3559&metric=duplicated_lines_density&view=list) No Duplication information
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] difin commented on a diff in pull request #3559: HIVE-26496,Improvement to fetch operator to scan only delete_delta fo…
Posted by GitBox <gi...@apache.org>.
difin commented on code in PR #3559:
URL: https://github.com/apache/hive/pull/3559#discussion_r971190432
##########
ql/src/java/org/apache/hadoop/hive/ql/io/orc/OrcSplit.java:
##########
@@ -104,28 +104,43 @@ public OrcSplit(Path path, Object fileId, long offset, long length, String[] hos
this.isOriginal = isOriginal;
this.hasBase = hasBase;
this.rootDir = rootDir;
- this.deltas.addAll(filterDeltasByBucketId(deltas, AcidUtils.parseBucketId(path)));
+ int bucketId = AcidUtils.parseBucketId(path);
+ AcidUtils.ParsedDeltaLight parentDelta = AcidUtils.ParsedDeltaLight.parse(getPath().getParent());
Review Comment:
Hi @deniskuzZ,
Many tests failed with the change of using AcidUtils.ParsedDeltaLight.parse() instead of AcidUtils.parseBaseOrDeltaBucketFilename(). As I understand the split is not always a delta folder, it can be some older format not supported by ParsedDeltaLight. I saw that ParsedDeltaLight.parse() is used in some cases internally in AcidUtils.parseBaseOrDeltaBucketFilename(), but not always. Can you please advise if I should revert to using AcidUtils.parseBaseOrDeltaBucketFilename() that worked in all cases or there is some better way?
[](https://github.com/apache/hive/blob/master/ql/src/java/org/apache/hadoop/hive/ql/io/AcidUtils.java#539)
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3559: HIVE-26496,Improvement to fetch operator to scan only delete_delta fo…
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3559:
URL: https://github.com/apache/hive/pull/3559#discussion_r971375969
##########
ql/src/java/org/apache/hadoop/hive/ql/io/orc/OrcSplit.java:
##########
@@ -104,13 +104,32 @@ public OrcSplit(Path path, Object fileId, long offset, long length, String[] hos
this.isOriginal = isOriginal;
this.hasBase = hasBase;
this.rootDir = rootDir;
- this.deltas.addAll(filterDeltasByBucketId(deltas, AcidUtils.parseBucketId(path)));
+ this.deltas.addAll(filterDeleteDeltasByWriteIds
+ (filterDeltasByBucketId(deltas, AcidUtils.parseBucketId(path)), conf));
this.projColsUncompressedSize = projectedDataSize <= 0 ? length : projectedDataSize;
// setting file length to Long.MAX_VALUE will let orc reader read file length from file system
this.fileLen = fileLen <= 0 ? Long.MAX_VALUE : fileLen;
this.syntheticAcidProps = syntheticAcidProps;
}
+ /**
+ * For every split we want to filter out the delete deltas that contain events that happened only
+ * in the past relative to the split
+ * @param deltas
+ * @param conf
+ * @return
+ */
+ protected List<AcidInputFormat.DeltaMetaData> filterDeleteDeltasByWriteIds(
+ List<AcidInputFormat.DeltaMetaData> deltas, Configuration conf) throws IOException {
+
+ AcidOutputFormat.Options orcSplitMinMaxWriteIds =
+ AcidUtils.parseBaseOrDeltaBucketFilename(getPath(), conf);
Review Comment:
root path could be either delta or base and we need to check if deleteDeltas are applicable to them. the prev version was actually correct
````
long minWriteId = !deltas.isEmpty() ?
AcidUtils.parseBaseOrDeltaBucketFilename(path, null).getMinimumWriteId() : -1;
this.deltas.addAll(
deltas.stream()
.filter(delta -> isQualifiedDeleteDeltasByWriteIds(delta, minWriteId))
.flatMap(delta -> filterDeltasByBucketId(delta, bucketId))
.collect(Collectors.toList()));
````
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org