Add read support for original files of ACID/Transactional tables #2930

harmandeeps · 2020-02-24T17:34:50Z

Fixes #2293.

Overview:

Original Files are created in Non-ACID table, but after it gets converted to an ACID table, these files persist until major compaction.
Original files don't have ACID columns, that's why to filter out the deleted rows (if someone deletes some rows after ACID conversion) by comparing the global row ID of a row with delete delta of the same bucket.

High-Level Approach:

For an original file split, filterout out all the original files/delete deltas belong to the same bucket. Send them in acidInfo.
For each original file split, calculate it's global start row ID which is the total number of rows present before the given original file in the same bucket.
In OrcDeletedRows, iterate from start row ID and filter out the deleted rows.

dain · 2020-03-23T18:15:29Z

I skimmed the code. The approach seems reasonable to me, but I'll leave the call to the others.

findepi

@harmandeeps
I did a first pass.

I need to yet fully understand how it works.
Is there any documentation covering delete delta with respect to original files?

findepi · 2020-06-19T12:42:19Z

presto-hive/src/main/java/io/prestosql/plugin/hive/AcidInfo.java

+    private final Optional<Integer> bucketId;
+
+    @JsonCreator
+    public AcidInfo(@JsonProperty("deleteDeltas") Optional<DeleteDeltaLocations> deleteDeltaLocations,


per code style, put each param on separate line

Suggested change

public AcidInfo(@JsonProperty("deleteDeltas") Optional<DeleteDeltaLocations> deleteDeltaLocations,

public AcidInfo(

@JsonProperty("deleteDeltas") Optional<DeleteDeltaLocations> deleteDeltaLocations,

findepi · 2020-06-19T12:46:49Z

presto-hive/src/main/java/io/prestosql/plugin/hive/AcidInfo.java

+    @Override
+    public boolean equals(Object o)
+    {
+        if (this == o) {
+            return true;
+        }
+        if (o == null || getClass() != o.getClass()) {
+            return false;
+        }
+
+        AcidInfo acidInfo = (AcidInfo) o;
+
+        if (!deleteDeltaLocations.equals(acidInfo.deleteDeltaLocations)) {
+            return false;
+        }
+        if (!originalFileLocations.equals(acidInfo.originalFileLocations)) {
+            return false;
+        }
+        return bucketId.equals(acidInfo.bucketId);
+    }
+
+    @Override
+    public int hashCode()
+    {
+        int result = deleteDeltaLocations.hashCode();
+        result = 31 * result + originalFileLocations.hashCode();
+        result = 31 * result + bucketId.hashCode();
+        return result;
+    }
+


I don't think AcidInfo needs to define equality. If so, please remove.

(Otherwise, use equals/hashCode generated by intellij)

findepi · 2020-06-19T12:48:05Z

presto-hive/src/main/java/io/prestosql/plugin/hive/AcidInfo.java

+    public boolean isSameBucket(Path path1, Path path2, Configuration conf)
+            throws IOException
+    {
+        if (bucketId.isPresent()) {
+            return bucketId.get() == AcidUtils.parseBaseOrDeltaBucketFilename(path2, conf).getBucketId();
+        }
+        return AcidUtils.parseBaseOrDeltaBucketFilename(path2, conf).getBucketId() ==
+                AcidUtils.parseBaseOrDeltaBucketFilename(path1, conf).getBucketId();
+    }


This method has some API problems, but fortunately it seems unused. Please remove.

findepi · 2020-06-19T12:48:43Z

presto-hive/src/main/java/io/prestosql/plugin/hive/AcidInfo.java

+        public Optional<DeleteDeltaLocations> getDeleteDeltaLocations()
+        {
+            return deleteDeltaLocations;
+        }
+
+        public Optional<OriginalFileLocations> getOriginalFileLocations()
+        {
+            return originalFileLocations;
+        }
+
+        public Optional<Integer> getBucketId()
+        {
+            return bucketId;
+        }


findepi · 2020-06-19T12:49:14Z

presto-hive/src/main/java/io/prestosql/plugin/hive/AcidInfo.java

+        public Builder addDeleteDeltaLocations(Optional<DeleteDeltaLocations> deleteDeltaLocations)
+        {
+            this.deleteDeltaLocations = deleteDeltaLocations;
+            return this;


this doesn't "add", so setDeleteDeltaLocations() would be better

findepi · 2020-06-19T14:45:44Z

presto-hive/src/main/java/io/prestosql/plugin/hive/orc/OrcPageSource.java

+    // Row ID relative to all the original files of the same bucket ID before this file in lexicographic order
+    private Optional<Long> originalFileRowId = Optional.empty();


Thanks for commenting here.

However, i do not fully understand. The original files do not have row ids, do they?

Yes, original files don't have row ids, but we need to calculate it to filter out the deleted rows as mentioned at line: L121.

findepi · 2020-06-19T14:46:48Z

presto-hive/src/main/java/io/prestosql/plugin/hive/orc/OrcPageSourceFactory.java

@@ -238,7 +240,7 @@ private static OrcPageSource createOrcPageSource(
            List<OrcColumn> fileReadColumns = new ArrayList<>(columns.size() + (isFullAcid ? 3 : 0));
            List<Type> fileReadTypes = new ArrayList<>(columns.size() + (isFullAcid ? 3 : 0));
            List<OrcReader.ProjectedLayout> fileReadLayouts = new ArrayList<>(columns.size() + (isFullAcid ? 3 : 0));
-            if (isFullAcid) {
+            if (isFullAcid && !originalFilesPresent) {


we should verify expected schema also when handling original files

findepi · 2020-06-19T14:46:51Z

presto-hive/src/main/java/io/prestosql/plugin/hive/orc/OrcPageSourceFactory.java

@@ -257,7 +259,7 @@ private static OrcPageSource createOrcPageSource(
            }

            Map<String, OrcColumn> fileColumnsByName = ImmutableMap.of();
-            if (useOrcColumnNames || isFullAcid) {
+            if (useOrcColumnNames || (isFullAcid && !originalFilesPresent)) {


we should verify expected schema also when handling original files

findepi · 2020-06-19T14:49:08Z

presto-hive/src/main/java/io/prestosql/plugin/hive/orc/OriginalFilesUtils.java

+            Path path = new Path(splitPath.getParent() + "/" + originalFileInfo.getName());
+            try {
+                // Check if the file belongs to the same bucket and comes before 'reqPath' in lexicographic order.
+                if (isSameBucket(splitPath, path, configuration) && path.compareTo(splitPath) < 0) {


since we BHSL divides information per bucket, do we need isSameBucket here?

yeah, it is redundant. removed.

findepi · 2020-06-19T14:54:29Z

presto-hive/src/main/java/io/prestosql/plugin/hive/orc/OrcDeleteDeltaPageSource.java

@@ -75,7 +75,8 @@ public OrcDeleteDeltaPageSource(
            String sessionUser,
            Configuration configuration,
            HdfsEnvironment hdfsEnvironment,
-            FileFormatDataSourceStats stats)
+            FileFormatDataSourceStats stats,
+            boolean originalFilesPresent)


This is not the right name. (And I am not sure about code structure too.)

This is because, when originalFilesPresent, this changes behavior of this class, in a incompatible matter.
So, data processing when there are no original files works some way A and when you create one orignal file, this class starts to beheave in a way B, which is not compatible with A.
For example, it discards transaction id form delta files.

Delete deltas for original files are different. I don't think we should discard data. We should verify there isn't the information we would be discarding.

If my understanding is correct, I suggest you rename originalFilesPresent -> originalFiles
and when set, validate the file does not have those transactionId, etc information we would be ignoring.

Delete deltas structure is same for deltas, base as well as original files. For delete deltas, we don't need to read bucket ID as we filter out delete delta with the same bucket as that of original file split (OrcDeletedRows.java -> getDeletedRows). We calculate original file row ID synthetically by reading the ORC footer of all the original files present before in lexicographically order + the location in the current ORC file.

So, there is no need to read original transaction ID, bucket ID from delta files to save read time.

So, there is no need to read original transaction ID, bucket ID from delta files to save read time.

I do not think it matters, and it makes the code more complex.
I view the original files as something we do not need to optimize for.
I think you think the same, otherwise we would perhaps be a bit smarter
than reading footer information from each of the files over and over again.

For this reason, please remove the originalFilesPresent from here, i.e.
let's back out changes from this class.

harmandeeps · 2020-06-21T12:13:18Z

@harmandeeps
I did a first pass.

I need to yet fully understand how it works.
Is there any documentation covering delete delta with respect to original files?

@findepi Thank you for the review. I will address the comments. There is no specific documentation that I found about original files. I read some Hive code, ORC structure to understand original files case. I have created a doc, please take a look: https://docs.google.com/document/d/1FeVu0kaunW3sg97Kr-be9WIWTgKKpybLo8ZvA5JBZvg/edit?usp=sharing

findepi · 2020-06-23T10:28:27Z

presto-hive/src/main/java/io/prestosql/plugin/hive/HiveSplit.java

@@ -50,7 +50,7 @@
    private final TableToPartitionMapping tableToPartitionMapping;
    private final Optional<BucketConversion> bucketConversion;
    private final boolean s3SelectPushdownEnabled;
-    private final Optional<DeleteDeltaLocations> deleteDeltaLocations;
+    private final Optional<AcidInfo> acidInfo;


Note: Would be nice to separate out a commit introducing AcidInfo class, separately from other changes.
You can do this on your own, or reuse first commit from #4049.

Yeah, I will create a separate commit introducing AcidInfo class.

it seems @wendigo took care of this in #4049. I think he picked the AcidInfo name to avoid causing conflicts with your PR, so you should just be able to rebase.

I have rebased PR on the master branch, thank you.

harmandeeps · 2020-06-29T13:58:37Z

@findepi : Thank you for the feedback. I have addressed your comments. Please have a look.

findepi

@harmandeeps thanks for your work on this.

findepi · 2020-07-01T12:13:05Z

presto-hive/src/main/java/io/prestosql/plugin/hive/AcidInfo.java

-    public AcidInfo(
-            @JsonProperty("partitionLocation") String partitionLocation,
-            @JsonProperty("deleteDeltas") List<DeleteDeltaInfo> deleteDeltas)
+    public AcidInfo(@JsonProperty("partitionLocation") String partitionLocation,


move argument to the next line

findepi · 2020-07-01T12:19:10Z

presto-hive/src/main/java/io/prestosql/plugin/hive/AcidInfo.java

-                deleteDeltas.equals(that.deleteDeltas);
+                deleteDeltas.equals(that.deleteDeltas) &&
+                originalFiles.equals(that.originalFiles) &&
+                bucketId == (that.bucketId);


please use intellij to generate equals

return bucketId == that.bucketId && Objects.equals(partitionLocation, that.partitionLocation) && Objects.equals(deleteDeltas, that.deleteDeltas) && Objects.equals(originalFiles, that.originalFiles);

findepi · 2020-07-01T12:20:12Z

presto-hive/src/main/java/io/prestosql/plugin/hive/AcidInfo.java

+    /**
+     * Stores original files related information.
+     * To calculate the correct starting row ID of an original file, OriginalFilesUtils needs OriginalFileInfo list.
+     */


Remove, this does not add more information than visible in class name and in OriginalFilesUtils signatures

findepi · 2020-07-01T12:20:56Z

presto-hive/src/main/java/io/prestosql/plugin/hive/AcidInfo.java

+        public OriginalFileInfo(@JsonProperty("name") String name,
+                @JsonProperty("fileSize") long fileSize)


Place all parameters on the same line, or each on separate.

findepi · 2020-07-01T12:27:23Z

presto-hive/src/main/java/io/prestosql/plugin/hive/AcidInfo.java

+                @JsonProperty("fileSize") long fileSize)
+        {
+            this.name = requireNonNull(name, "name is null");
+            checkArgument(fileSize > 0, "fileSize should be > 0");


Empty files are legal ORC files. Please add a test covering that.

presto-hive/src/main/java/io/prestosql/plugin/hive/BackgroundHiveSplitLoader.java

findepi · 2020-07-01T13:16:15Z

presto-hive/src/main/java/io/prestosql/plugin/hive/BackgroundHiveSplitLoader.java

+        }
+    }
+
+    private void generateOriginalFilesSplits(Configuration configuration, FileSystem fs, InternalHiveSplitFactory splitFactory,


please each parameter on a separate line

findepi · 2020-07-01T13:16:36Z

presto-hive/src/main/java/io/prestosql/plugin/hive/BackgroundHiveSplitLoader.java

+    private void generateOriginalFilesSplits(Configuration configuration, FileSystem fs, InternalHiveSplitFactory splitFactory,
+            List<HadoopShims.HdfsFileStatusWithId> originalFileLocations, Optional<AcidInfo.Builder> acidInfoBuilder)
+    {
+        if (originalFileLocations == null || originalFileLocations.isEmpty()) {


originalFileLocations cannot be null nor empty here

findepi · 2020-07-01T13:17:55Z

presto-hive/src/main/java/io/prestosql/plugin/hive/BackgroundHiveSplitLoader.java

+        // INSERT ONLY Tables case
+        if (!acidInfoBuilder.isPresent()) {
+            addOriginalFilesUtil(
+                    configuration,
+                    fs,
+                    originalFileLocations,
+                    splitFactory,
+                    Optional.empty());
+            return;
+        }


this is dead code, right?

findepi · 2020-07-01T13:18:16Z

presto-hive/src/main/java/io/prestosql/plugin/hive/BackgroundHiveSplitLoader.java

+    }
+
+    private void generateOriginalFilesSplits(Configuration configuration, FileSystem fs, InternalHiveSplitFactory splitFactory,
+            List<HadoopShims.HdfsFileStatusWithId> originalFileLocations, Optional<AcidInfo.Builder> acidInfoBuilder)


acidInfoBuilder is always present, does not need to be Optional

harmandeeps

@findepi : Thank you for the review, I have addressed the comments. Please take a look.

presto-hive/src/main/java/io/prestosql/plugin/hive/BackgroundHiveSplitLoader.java

harmandeeps · 2020-07-06T13:03:08Z

presto-hive/src/main/java/io/prestosql/plugin/hive/AcidInfo.java

+/*            Path partitionPathFromOriginalPath = originalFilePath.getParent();
+            checkArgument(
+                    partitionLocation.equals(partitionPathFromOriginalPath),
+                    "Partition location in OriginalFile '%s' does not match stored location '%s'",
+                    originalFilePath.getParent().toString(),
+                    partitionLocation);*/


The product tests were failing because of error:

2020-06-29T14:25:20.2138260Z tests | Caused by: java.lang.IllegalArgumentException: Partition location in OriginalFile '/user/hive/warehouse/reading_full_acid_converted_table' does not match stored location 'hdfs://hadoop-master:9000/user/hive/warehouse/reading_full_acid_converted_table' 2020-06-29T14:25:20.2138423Z tests | at com.google.common.base.Preconditions.checkArgument(Preconditions.java:441) 2020-06-29T14:25:20.2138556Z tests | at io.prestosql.plugin.hive.AcidInfo$Builder.addOriginalFile(AcidInfo.java:288)

presto-hive/src/main/java/io/prestosql/plugin/hive/AcidInfo.java

presto-hive/src/main/java/io/prestosql/plugin/hive/BackgroundHiveSplitLoader.java

harmandeeps · 2020-07-06T15:18:41Z

presto-hive/src/main/java/io/prestosql/plugin/hive/orc/OrcDeletedRows.java

@@ -193,7 +206,12 @@ private void loadValidPositions()
                        Page page = pageSource.getNextPage();
                        if (page != null) {
                            for (int i = 0; i < page.getPositionCount(); i++) {
-                                deletedRowsBuilder.add(new RowId(page, i));
+                                long originalTransaction = -1;


@findepi : Here, we filter the delete delta on bucketId first, therefore, there is no need of reading bucketId from ACID columns. If this approach looks fine, then I can push changes to remove 'bucket' variable from RowId class.

Let's not complicate this PR. To be considered later, OK?

presto-hive/src/main/java/io/prestosql/plugin/hive/BackgroundHiveSplitLoader.java

findepi · 2020-07-17T11:51:08Z

presto-hive/src/main/java/io/prestosql/plugin/hive/AcidInfo.java

+/*            Path partitionPathFromOriginalPath = originalFilePath.getParent();
+            checkArgument(
+                    partitionLocation.equals(partitionPathFromOriginalPath),
+                    "Partition location in OriginalFile '%s' does not match stored location '%s'",
+                    originalFilePath.getParent().toString(),
+                    partitionLocation);*/


Is it related to a05d92b#r448353960?

in the worst case, you can compare "path's uri's path" only
and add a comment explaining why doing so

findepi · 2020-07-17T11:53:04Z

presto-hive/src/main/java/io/prestosql/plugin/hive/orc/OrcDeletedRows.java

@@ -193,7 +206,12 @@ private void loadValidPositions()
                        Page page = pageSource.getNextPage();
                        if (page != null) {
                            for (int i = 0; i < page.getPositionCount(); i++) {
-                                deletedRowsBuilder.add(new RowId(page, i));
+                                long originalTransaction = -1;


Let's not complicate this PR. To be considered later, OK?

presto-hive/src/main/java/io/prestosql/plugin/hive/AcidInfo.java

presto-hive/src/main/java/io/prestosql/plugin/hive/BackgroundHiveSplitLoader.java

presto-hive/src/test/java/io/prestosql/plugin/hive/TestOriginalFilesUtils.java

...s/src/main/java/io/prestosql/tests/hive/acid/TestFlatTableConvertedToTransactionalTable.java

presto-hive/src/main/java/io/prestosql/plugin/hive/BackgroundHiveSplitLoader.java

findepi · 2020-07-17T12:59:49Z

@electrum please double review BackgroundHiveSplitLoader changes in the case when original files are not present.

losipiuk · 2020-07-20T11:09:23Z

presto-hive/src/main/java/io/prestosql/plugin/hive/AcidInfo.java

+                    "entry for requested bucket Id");
+            List<DeleteDeltaInfo> deleteDeltas = deleteDeltaInfoBuilder.build();
+            if (deleteDeltas.isEmpty()) {
+                return Optional.empty();


With current shape of this PR Presto fails to read table data if there are original files and there are no delete_delta files present.
If (deleteDeltas.isEmpty()) resolves to true we are loosing all the original files information. Yet Orc reader, later on, expects the ACID columns to be present in data files.
This results in following exception on read:

Query 20200720_104400_00016_v7svz failed: ORC ACID file should have 6 columns: hdfs://hadoop-master:9000/user/hive/warehouse/t/000000_0 io.prestosql.spi.PrestoException: ORC ACID file should have 6 columns: hdfs://hadoop-master:9000/user/hive/warehouse/t/000000_0 at io.prestosql.plugin.hive.orc.OrcPageSourceFactory.verifyAcidSchema(OrcPageSourceFactory.java:418) at io.prestosql.plugin.hive.orc.OrcPageSourceFactory.createOrcPageSource(OrcPageSourceFactory.java:244) at io.prestosql.plugin.hive.orc.OrcPageSourceFactory.createPageSource(OrcPageSourceFactory.java:160) at io.prestosql.plugin.hive.HivePageSourceProvider.createHivePageSource(HivePageSourceProvider.java:178) at io.prestosql.plugin.hive.HivePageSourceProvider.createPageSource(HivePageSourceProvider.java:105) at io.prestosql.plugin.base.classloader.ClassLoaderSafeConnectorPageSourceProvider.createPageSource(ClassLoaderSafeConnectorPageSourceProvider.java:57) at io.prestosql.split.PageSourceManager.createPageSource(PageSourceManager.java:64) at io.prestosql.operator.TableScanOperator.getOutput(TableScanOperator.java:298) at io.prestosql.operator.Driver.processInternal(Driver.java:379) at io.prestosql.operator.Driver.lambda$processFor$8(Driver.java:283) at io.prestosql.operator.Driver.tryWithLock(Driver.java:675) at io.prestosql.operator.Driver.processFor(Driver.java:276) at io.prestosql.execution.SqlTaskExecution$DriverSplitRunner.processFor(SqlTaskExecution.java:1075) at io.prestosql.execution.executor.PrioritizedSplitRunner.process(PrioritizedSplitRunner.java:163) at io.prestosql.execution.executor.TaskExecutor$TaskRunner.run(TaskExecutor.java:484) at io.prestosql.$gen.Presto_unknown____20200720_103058_2.run(Unknown Source) at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128) at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628) at java.base/java.lang.Thread.run(Thread.java:834)

Repro steps:
Create non-transactional table on hive and add some data:

CREATE TABLE t (a integer) stored as orc tblproperties ('transactional'='false'); insert into t values (1);

make table transactional:

ALTER TABLE t SET TBLPROPERTIES ('transactional'='true');

At this point reads from presto fail. To make them work we currently need to delete something from table t, so delete_delta file is present.

@losipiuk : Thank you for pointing this out, will fix this.

diff --git a/presto-hive/src/main/java/io/prestosql/plugin/hive/AcidInfo.java b/presto-hive/src/main/java/io/prestosql/plugin/hive/AcidInfo.java index 15838b165b..14f8d8d580 100644 --- a/presto-hive/src/main/java/io/prestosql/plugin/hive/AcidInfo.java +++ b/presto-hive/src/main/java/io/prestosql/plugin/hive/AcidInfo.java @@ -48,7 +48,6 @@ public class AcidInfo { this.partitionLocation = requireNonNull(partitionLocation, "partitionLocation is null"); this.deleteDeltas = ImmutableList.copyOf(requireNonNull(deleteDeltas, "deleteDeltas is null")); - checkArgument(!deleteDeltas.isEmpty(), "deleteDeltas is empty"); this.originalFiles = ImmutableList.copyOf(requireNonNull(originalFiles, "originalFiles is null")); this.bucketId = bucketId; } @@ -296,9 +295,6 @@ public class AcidInfo checkState(bucketId > -1 && bucketIdToOriginalFileInfoMap.containsKey(bucketId), "Bucket Id to OriginalFileInfo map should have " + "entry for requested bucket Id"); List<DeleteDeltaInfo> deleteDeltas = deleteDeltaInfoBuilder.build(); - if (deleteDeltas.isEmpty()) { - return Optional.empty(); - } return Optional.of(new AcidInfo(partitionLocation.toString(), deleteDeltas, bucketIdToOriginalFileInfoMap.get(bucketId), bucketId)); }

seems to improve situation, yet I am not sure if it does not break some flow.

harmandeeps

Thank you for the review @findepi . I have addressed the comments, please take a look.

presto-hive/src/main/java/io/prestosql/plugin/hive/BackgroundHiveSplitLoader.java

harmandeeps · 2020-07-22T09:08:26Z

presto-hive/src/main/java/io/prestosql/plugin/hive/BackgroundHiveSplitLoader.java

+        }
+
+        if (!fileStatusOriginalFiles.isEmpty()) {
+            generateOriginalFilesSplits(fs, splitFactory, fileStatusOriginalFiles, acidInfoBuilder);


we return at L531 in that case, so it won't be executed.

harmandeeps · 2020-07-22T09:09:14Z

presto-hive/src/test/java/io/prestosql/plugin/hive/TestHiveSplit.java

@@ -55,6 +55,10 @@ public void testJsonRoundTrip()
        AcidInfo.Builder acidInfoBuilder = AcidInfo.builder(new Path("file:///data/fullacid"));
        acidInfoBuilder.addDeleteDelta(new Path("file:///data/fullacid/delete_delta_0000004_0000004_0000"), 4L, 4L, 0);
        acidInfoBuilder.addDeleteDelta(new Path("file:///data/fullacid/delete_delta_0000007_0000007_0000"), 7L, 7L, 0);
+
+        acidInfoBuilder.addOriginalFile(new Path("file:///data/fullacid/000000_0"), 120, 0);
+        acidInfoBuilder.addOriginalFile(new Path("file:///data/fullacid/000001_0"), 125, 0);


We need this to compare the acidInfo.

harmandeeps · 2020-07-22T09:16:40Z

presto-hive/src/main/java/io/prestosql/plugin/hive/orc/OrcDeleteDeltaPageSource.java

                    acidColumns.get(ACID_COLUMN_ORIGINAL_TRANSACTION.toLowerCase(ENGLISH)),
-                    acidColumns.get(ACID_COLUMN_BUCKET.toLowerCase(ENGLISH)),


We need <originalTransaction, bucketId, rowId> to correctly figure out if a row is deleted.

In case of original files, we don't have ACID columns. We calculate rowId as given in OriginalFilesUtils, originalTrasaction is always 0. But, we don't know the bucketId. So, for faster read and simplification, we can drop not reading bucketId at all in this PR. That's why, I have dropped this Bucket column read code. Please let me know your take on this.

harmandeeps · 2020-07-22T09:17:13Z

presto-hive/src/main/java/io/prestosql/plugin/hive/orc/OrcDeleteDeltaPageSource.java

                    acidColumns.get(ACID_COLUMN_ROW_ID.toLowerCase(ENGLISH)));

            recordReader = reader.createRecordReader(
                    rowIdColumns,
-                    ImmutableList.of(BIGINT, INTEGER, BIGINT),


as explained above, to avoid reading the bucket id column.

harmandeeps · 2020-07-22T09:19:44Z

presto-hive/src/main/java/io/prestosql/plugin/hive/orc/OrcDeletedRows.java

+                                int bucket = -1;
+                                long row;
+                                originalTransaction = BIGINT.getLong(page.getBlock(ORIGINAL_TRANSACTION_INDEX), i);
+                                row = BIGINT.getLong(page.getBlock(BUCKET_ID_INDEX), i);


As now, we are reading only originalTxnId, rowId. So, we should read index 0, 1. As,
BUCKET_ID_INDEX = 1, so I used it. I will change the variable value if this approach looks fine.

harmandeeps · 2020-07-22T11:56:10Z

presto-hive/src/test/java/io/prestosql/plugin/hive/TestHiveSplit.java

@@ -55,10 +55,6 @@ public void testJsonRoundTrip()
        AcidInfo.Builder acidInfoBuilder = AcidInfo.builder(new Path("file:///data/fullacid"));
        acidInfoBuilder.addDeleteDelta(new Path("file:///data/fullacid/delete_delta_0000004_0000004_0000"), 4L, 4L, 0);


if we are keeping delete delta, why are we removing original file? am I missing something?

i backed out a change in the file, which seemed unnecessary (test passed with and without).
however, i would be more happy to understand why we want it here.

I guess the notion was to check if deserialization of HiveSplit works fine. It may help to catch the errors when some issue with Json serialization and we do have Json params in OriginalFileLocations.

I like the idea. It needs to be reflected in the test assertions too. Can you please address this as a follow up PR?

yeah, will take this up as a follow up PR.

harmandeeps

Thank you for the review @losipiuk . I have addressed the comments.

harmandeeps · 2020-07-23T07:39:00Z

presto-hive/src/main/java/io/prestosql/plugin/hive/orc/OrcPageSource.java

@@ -108,8 +114,11 @@ public Page getNextPage()
            return null;
        }

+        OptionalLong startRowId = originalFileRowId.isPresent() ?


I guess the present one is more readable. Otherwise, it was getting messy as to convert Optional<> to OptionalInt in case if it present.

harmandeeps · 2020-07-23T07:39:24Z

presto-hive/src/test/java/io/prestosql/plugin/hive/orc/TestOrcDeletedRows.java

+        AcidInfo.Builder acidInfoBuilder = AcidInfo.builder(path);
+        addDeleteDelta(acidInfoBuilder, 10000001L, 10000001L, 0, path);
+
+        acidInfoBuilder.addOriginalFile(new Path(path, "000000_0"), 743, 0);


harmandeeps · 2020-07-23T07:39:31Z

presto-product-tests/src/main/java/io/prestosql/tests/hive/TestHiveTransactionalTable.java

@@ -188,6 +188,83 @@ public void testReadInsertOnly(boolean isPartitioned, BucketingType bucketingTyp
        }
    }

+    @Test(groups = {STORAGE_FORMATS, HIVE_TRANSACTIONAL}, dataProvider = "partitioningAndBucketingTypeDataProvider", timeOut = TEST_TIMEOUT)


harmandeeps · 2020-07-23T07:40:27Z

presto-hive/src/main/java/io/prestosql/plugin/hive/orc/OriginalFilesUtils.java

+            FileFormatDataSourceStats stats)
+    {
+        long rowCount = 0;
+        for (OriginalFileInfo originalFileInfo : originalFileInfos) {


If we can come up with a cache size and eviction time, then we can do this too.

losipiuk · 2020-07-23T08:11:40Z

presto-hive/src/main/java/io/prestosql/plugin/hive/orc/OrcDeletedRows.java

 import static java.util.Objects.requireNonNull;
 import static org.apache.hadoop.hive.ql.io.AcidUtils.deleteDeltaSubdir;

 @NotThreadSafe
 public class OrcDeletedRows
 {
+    private static final int ORIGINAL_TRANSACTION_INDEX = 0;
+    private static final int ROW_ID_INDEX_DELETE_DELTA = 1; // delete delta page has columns: <originalId,rowId>
+    private static final int ROW_ID_INDEX = 2; // base/delta page has columns: <originalId,bucketId,rowId>


If we are do not care about bucket id from base/delta pages can't we just refrain from setting up PageSource from reading it.

I.e. drop this

fileReadColumns.add(acidColumnsByName.get(ACID_COLUMN_BUCKET.toLowerCase(ENGLISH))); fileReadTypes.add(INTEGER); fileReadLayouts.add(fullyProjectedLayout());

from https://github.com/prestosql/presto/blob/c0ead8f19be0930a5ad820033de77a34dd9fe125/presto-hive/src/main/java/io/prestosql/plugin/hive/orc/OrcPageSourceFactory.java#L192 ?

Then we could use same index. Right?

yeah, I have pushed the changes.

presto-product-tests/src/main/java/io/prestosql/tests/hive/TestHiveTransactionalTable.java

harmandeeps · 2020-07-23T15:13:51Z

presto-product-tests/src/main/java/io/prestosql/tests/hive/TestHiveTransactionalTable.java

@@ -97,18 +97,6 @@ public void testReadFullAcidBucketedV2()
        doTestReadFullAcid(false, BucketingType.BUCKETED_V2);
    }

-    @Test(groups = HIVE_TRANSACTIONAL, timeOut = TEST_TIMEOUT)


@findepi : @losipiuk suggested me to add product tests for bucket version v1/v2.

i am aware. but they were not correct

findepi · 2020-07-24T13:33:22Z

CI failed -- #4395

findepi · 2020-07-24T15:27:28Z

Merged as 62e1622, thanks!

cla-bot bot added the cla-signed label Feb 24, 2020

harmandeeps requested review from shubhamtagra, findepi and dain February 24, 2020 17:35

harmandeeps requested a review from sopel39 March 3, 2020 10:33

dain removed their request for review March 23, 2020 18:14

harmandeeps force-pushed the read_support_acid_original_files branch 2 times, most recently from c44c930 to 9e9d1a4 Compare June 16, 2020 05:11

findepi reviewed Jun 19, 2020

View reviewed changes

harmandeeps requested a review from findepi June 22, 2020 06:56

findepi reviewed Jun 23, 2020

View reviewed changes

harmandeeps force-pushed the read_support_acid_original_files branch 2 times, most recently from ebc0c9c to a05d92b Compare June 29, 2020 13:55

findepi reviewed Jul 1, 2020

View reviewed changes

harmandeeps force-pushed the read_support_acid_original_files branch 2 times, most recently from c01b7d3 to 7546748 Compare July 6, 2020 18:23

harmandeeps commented Jul 6, 2020

View reviewed changes

harmandeeps force-pushed the read_support_acid_original_files branch 3 times, most recently from 97d243e to ee88d97 Compare July 8, 2020 13:52

findepi reviewed Jul 17, 2020

View reviewed changes

losipiuk reviewed Jul 20, 2020

View reviewed changes

harmandeeps commented Jul 22, 2020

View reviewed changes

harmandeeps force-pushed the read_support_acid_original_files branch from 2112140 to 9af7669 Compare July 22, 2020 09:57

findepi mentioned this pull request Jul 22, 2020

Support CTAS for Hive transactional tables #4516

Closed

harmandeeps commented Jul 22, 2020

View reviewed changes

findepi added 8 commits July 22, 2020 22:36

reorder methods

b7cc23e

review fixes

4732906

review fixes

f829d83

Remove createInternalHiveSplit and fix splittable

e33f8c0

Refac fileIterators usage

284058d

Fix original files with filter

e371387

review fixes

e41d2e6

fixes to fixes

239591d

harmandeeps force-pushed the read_support_acid_original_files branch 2 times, most recently from c09de52 to a2f81c5 Compare July 22, 2020 20:28

fixing product test and unit test

ee1c88f

harmandeeps force-pushed the read_support_acid_original_files branch from a2f81c5 to ee1c88f Compare July 23, 2020 04:13

addressing review comments

c0ead8f

harmandeeps commented Jul 23, 2020

View reviewed changes

losipiuk reviewed Jul 23, 2020

View reviewed changes

Harmandeep Singh and others added 6 commits July 23, 2020 18:07

dont read ACID bucket ID column for deltas

02d189e

Fix .containsExactly -> .containsOnly and remove redundant ORDER BY

b6c0ff2

Simplify per LO comment

c2086e7

Fix fmt

16245d5

Add TODO comment

ec99fb9

Remove bogus original files + bucketing v1/2 tests

2ddb40a

harmandeeps commented Jul 23, 2020

View reviewed changes

presto-product-tests/src/main/java/io/prestosql/tests/hive/TestHiveTransactionalTable.java Outdated Show resolved Hide resolved

harmandeeps commented Jul 23, 2020

View reviewed changes

findepi added 2 commits July 23, 2020 17:55

empty commit for Ci

eb1c80c

empty commit for Ci

5f688b0

findepi closed this Jul 24, 2020

findepi added this to the 340 milestone Jul 24, 2020

findepi mentioned this pull request Jul 24, 2020

Release notes for 340 #4527

Closed

8 tasks

	public AcidInfo(@JsonProperty("deleteDeltas") Optional<DeleteDeltaLocations> deleteDeltaLocations,
	public AcidInfo(
	@JsonProperty("deleteDeltas") Optional<DeleteDeltaLocations> deleteDeltaLocations,

		// Row ID relative to all the original files of the same bucket ID before this file in lexicographic order
		private Optional<Long> originalFileRowId = Optional.empty();

		public OriginalFileInfo(@JsonProperty("name") String name,
		@JsonProperty("fileSize") long fileSize)

		acidColumns.get(ACID_COLUMN_ORIGINAL_TRANSACTION.toLowerCase(ENGLISH)),
		acidColumns.get(ACID_COLUMN_BUCKET.toLowerCase(ENGLISH)),

		@@ -55,10 +55,6 @@ public void testJsonRoundTrip()
		AcidInfo.Builder acidInfoBuilder = AcidInfo.builder(new Path("file:///data/fullacid"));
		acidInfoBuilder.addDeleteDelta(new Path("file:///data/fullacid/delete_delta_0000004_0000004_0000"), 4L, 4L, 0);

Add read support for original files of ACID/Transactional tables #2930

Add read support for original files of ACID/Transactional tables #2930

Conversation

harmandeeps commented Feb 24, 2020 • edited Loading

dain commented Mar 23, 2020

findepi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harmandeeps commented Jun 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harmandeeps commented Jun 29, 2020

findepi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harmandeeps left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

findepi commented Jul 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harmandeeps left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harmandeeps left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

losipiuk Jul 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harmandeeps Jul 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harmandeeps commented Feb 24, 2020 •

edited

Loading

losipiuk Jul 23, 2020 •

edited

Loading

harmandeeps Jul 23, 2020 •

edited

Loading