Fix inserting into transactional table when task_writer_count > 1 (v2) #10460

homar · 2022-01-04T15:54:56Z

fixes: #9149

core/trino-main/src/main/java/io/trino/sql/planner/optimizations/StreamPropertyDerivations.java

plugin/trino-hive/src/main/java/io/trino/plugin/hive/HivePageSink.java

plugin/trino-hive/src/test/java/io/trino/plugin/hive/AbstractTestHive.java

core/trino-main/src/main/java/io/trino/sql/planner/optimizations/StreamPropertyDerivations.java

plugin/trino-hive/src/test/java/io/trino/plugin/hive/AbstractTestHive.java

findepi · 2022-01-05T12:06:27Z

@sopel39 do you want to take a look?

sopel39 · 2022-01-05T12:18:32Z

@sopel39 do you want to take a look?

Yes, I will

homar · 2022-01-10T14:12:14Z

@sopel39 any chances you will find some time to take a look this week?

sopel39 · 2022-01-10T16:46:41Z

core/trino-main/src/main/java/io/trino/sql/planner/optimizations/StreamPropertyDerivations.java

@@ -338,6 +338,10 @@ public StreamProperties visitExchange(ExchangeNode node, List<StreamProperties>
                    if (node.getPartitioningScheme().getPartitioning().getHandle().equals(FIXED_ARBITRARY_DISTRIBUTION)) {
                        return new StreamProperties(FIXED, Optional.empty(), false);
                    }
+                    // empty arguments list means the bucketing function is effectively constant (1 bucket)


what about PropertyDerivations.Visitor#visitExchange?

sopel39 · 2022-01-10T16:47:25Z

core/trino-main/src/main/java/io/trino/sql/planner/optimizations/StreamPropertyDerivations.java

@@ -338,6 +338,10 @@ public StreamProperties visitExchange(ExchangeNode node, List<StreamProperties>
                    if (node.getPartitioningScheme().getPartitioning().getHandle().equals(FIXED_ARBITRARY_DISTRIBUTION)) {
                        return new StreamProperties(FIXED, Optional.empty(), false);
                    }
+                    // empty arguments list means the bucketing function is effectively constant (1 bucket)
+                    if (node.getPartitioningScheme().getPartitioning().getArguments().isEmpty()) {
+                        return new StreamProperties(SINGLE, Optional.of(ImmutableSet.of()), false);


there is a shortcut for it: StreamPropertyDerivations.StreamProperties#singleStream

sopel39 · 2022-01-10T16:56:37Z

core/trino-main/src/main/java/io/trino/sql/planner/optimizations/StreamPropertyDerivations.java

@@ -338,6 +338,10 @@ public StreamProperties visitExchange(ExchangeNode node, List<StreamProperties>
                    if (node.getPartitioningScheme().getPartitioning().getHandle().equals(FIXED_ARBITRARY_DISTRIBUTION)) {
                        return new StreamProperties(FIXED, Optional.empty(), false);
                    }
+                    // empty arguments list means the bucketing function is effectively constant (1 bucket)
+                    if (node.getPartitioningScheme().getPartitioning().getArguments().isEmpty()) {


I'm not sure this always works.
For example LocalExecutionPlanner.Visitor#visitTableWriter sets:

context.setDriverInstanceCount(getTaskWriterCount(session));

while LocalExecutionPlanner.Visitor#createMergeSource sets:

context.setDriverInstanceCount(1);

This would fail if such operators are part of same pipeline. We use StreamProperties to add local exchanges which split single large pipeline into separate ones.

Perhaps this works in your use case, but I think this breaks definition of stream properties. You need exactly one stream (set via setDriverInstanceCount(1)) to be able to use singleStream.

I think it's also wrong because connector could return some random partitioning even if there are no arguments.
I would just skip property changes in this PR

there is unwritten, pre-existing assumption that partitioning must be deterministic. it doesn't seem to make sense otherwise

It doesn't have to be deterministic, see io.trino.sql.planner.SystemPartitioningHandle.SystemPartitionFunction.RoundRobinBucketFunction

This would fail if such operators are part of same pipeline.

Can you think of any scenario that would actually create such a pipeline?

I would just skip property changes in this PR

Unfortunately changing this property is the core of this pr - at least I don't see any other ways to do this.

Ok but as you can see in this PR tests also passed yet you claim this is incorrect why trusting tests then and not now ?

I've described why this PR breaks relationship between properties and local execution planner (#10460 (comment)).

I don't think execution model enforces

checkArgument(distribution == SINGLE || !this.partitioningColumns.equals(Optional.of(ImmutableList.of())), "Multiple streams must not be partitioned on empty set");

(for example on global level it's fine to have non-deterministic partitioning with empty set).
I think that check was just added a bit eagerly.

(for example on global level it's fine to have non-deterministic partitioning with empty set).

that doesn't make sense to me as a feature. why would connector return a partitioning at all?

and, i don't accept this as a design decision. Determinism is an important aspect, and not to be given away without good reason.

and, i don't accept this as a design decision. Determinism is an important aspect, and not to be given away without good reason.

One could imagine partitioning that is based on load (e.g when writing data from one system to the other). It doesn't have to be connector provided (it could be system partitioning, just using SPI)

I removed the check

sopel39 · 2022-01-10T16:57:40Z

plugin/trino-hive-hadoop2/src/test/java/io/trino/plugin/hive/TestHive.java

@@ -60,6 +60,20 @@ private int getHiveVersionMajor()
        return hiveVersionMajor;
    }

+    @Test
+    public void testInsertBucketedTransactionalTableLayout()


I missed the explanation why this cant be in AbstractTestHive while insertBucketedTableLayout(false) and insertPartitionedBucketedTableLayout(false) are

Because if they are in AbstractTestHive then TestHiveAlluxioMetastore fails because of them.

Because if they are in AbstractTestHive then TestHiveAlluxioMetastore fails because of them.

Add them to AbstractTestHive and override them in TestHiveAlluxioMetastore with a comment why they don't work

Ok I will do that, but I am not sure why this is a better way.

Ok I will do that, but I am not sure why this is a better way.

We use this approach in other abstract/dervied tests

fixes: trinodb#9149

sopel39 · 2022-01-13T09:23:25Z

plugin/trino-hive/src/main/java/io/trino/plugin/hive/HivePageSink.java

-                if (bucketFunction != null || writer.getWrittenBytes() <= targetMaxFileSize.orElse(Long.MAX_VALUE)) {
+                // for transactional tables we don't want to split output files because there is an explicit or implicit bucketing
+                // and file names have no random component (e.g. bucket_00000)
+                if (bucketFunction != null || isTransactional || writer.getWrittenBytes() <= targetMaxFileSize.orElse(Long.MAX_VALUE)) {


Isn't bucket function always present when isTransactional=true?

It is not present in the scenario I am trying to fix

cla-bot bot added the cla-signed label Jan 4, 2022

homar added the tests:hive label Jan 4, 2022

homar force-pushed the homar/insert_into_unbucketed_trans_table_when_writer_task_v2 branch 2 times, most recently from 734a988 to 1c78a61 Compare January 4, 2022 16:20

findepi changed the title ~~[WIP] Fix inserting into transactional table when task_writer_count > 1~~ Fix inserting into transactional table when task_writer_count > 1 (v2) Jan 4, 2022

homar force-pushed the homar/insert_into_unbucketed_trans_table_when_writer_task_v2 branch 3 times, most recently from 240fd65 to 5531384 Compare January 4, 2022 23:17

findepi marked this pull request as ready for review January 5, 2022 07:59

findepi reviewed Jan 5, 2022

View reviewed changes

homar force-pushed the homar/insert_into_unbucketed_trans_table_when_writer_task_v2 branch from 5531384 to 70464bc Compare January 5, 2022 09:54

findepi approved these changes Jan 5, 2022

View reviewed changes

homar force-pushed the homar/insert_into_unbucketed_trans_table_when_writer_task_v2 branch 2 times, most recently from 01a7585 to 43a8ff0 Compare January 5, 2022 12:01

findepi approved these changes Jan 5, 2022

View reviewed changes

sopel39 reviewed Jan 10, 2022

View reviewed changes

sopel39 mentioned this pull request Jan 11, 2022

Fix inserting into transactional table when task_writer_count > 1 #10261

Closed

Fix inserting into transactional table when task_writer_count > 1

58e0c34

fixes: trinodb#9149

homar force-pushed the homar/insert_into_unbucketed_trans_table_when_writer_task_v2 branch from 43a8ff0 to 58e0c34 Compare January 12, 2022 15:16

sopel39 approved these changes Jan 13, 2022

View reviewed changes

findepi merged commit 20a38b0 into trinodb:master Jan 13, 2022

findepi mentioned this pull request Jan 13, 2022

Release notes for 369 #10552

Closed

github-actions bot added this to the 369 milestone Jan 13, 2022

mosabua mentioned this pull request Jan 13, 2022

Add Trino 369 release notes #10553

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix inserting into transactional table when task_writer_count > 1 (v2) #10460

Fix inserting into transactional table when task_writer_count > 1 (v2) #10460

homar commented Jan 4, 2022

findepi commented Jan 5, 2022

sopel39 commented Jan 5, 2022

homar commented Jan 10, 2022 •

edited

sopel39 Jan 10, 2022

sopel39 Jan 10, 2022

sopel39 Jan 10, 2022

sopel39 Jan 10, 2022

findepi Jan 11, 2022

sopel39 Jan 11, 2022

homar Jan 11, 2022

homar Jan 12, 2022

sopel39 Jan 12, 2022 •

edited

findepi Jan 12, 2022

sopel39 Jan 12, 2022

homar Jan 12, 2022

sopel39 Jan 10, 2022

homar Jan 11, 2022

sopel39 Jan 11, 2022 •

edited

homar Jan 11, 2022

sopel39 Jan 11, 2022

sopel39 Jan 13, 2022

homar Jan 13, 2022

Fix inserting into transactional table when task_writer_count > 1 (v2) #10460

Fix inserting into transactional table when task_writer_count > 1 (v2) #10460

Conversation

homar commented Jan 4, 2022

findepi commented Jan 5, 2022

sopel39 commented Jan 5, 2022

homar commented Jan 10, 2022 • edited

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

sopel39 Jan 12, 2022 • edited

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

sopel39 Jan 11, 2022 • edited

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

homar commented Jan 10, 2022 •

edited

sopel39 Jan 12, 2022 •

edited

sopel39 Jan 11, 2022 •

edited