Fix the explain (TYPE IO) Exception when table is hive partition tabl… #12349

albericgenius · 2022-05-12T08:32:07Z

Description

Fix the explain (TYPE IO) Exception when table is hive partition table which is empty

Is this change a fix, improvement, new feature, refactoring, or other?
Yes, a bug fix
Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)
This a change for HIVE connector
How would you describe this change to a non-technical end user or system administrator?
Fix the explain (TYPE IO) Exception when table is hive partition table which is empty

Related issues, pull requests, and links

#10398

Documentation

(+) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

(+) No release notes entries required.
( ) Release notes entries required with the following suggested text:

# Section
* Fix some things. ({issue}`10398`)

findinpath · 2022-05-13T04:51:42Z

Please do follow the guideline for the Git commits

https://github.com/trinodb/trino/blob/master/.github/DEVELOPMENT.md#format-git-commit-messages

Make the git commit message header much smaller, wrap the message to 70-80 chars, and concentrate on make it more descriptive for the other fellows working on your changes.

findinpath · 2022-05-13T04:55:49Z

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

@@ -2926,9 +2926,12 @@ public ConnectorTableHandle makeCompatiblePartitioning(ConnectorSession session,
    @VisibleForTesting
    static TupleDomain<ColumnHandle> createPredicate(List<ColumnHandle> partitionColumns, List<HivePartition> partitions)
    {
-        if (partitions.isEmpty()) {
+        if (partitionColumns.isEmpty() && partitions.isEmpty()) {


Suggested change

if (partitionColumns.isEmpty() && partitions.isEmpty()) {

if (partitions.isEmpty()){

return partitionColumns.isEmpty() ? TupleDomain.none() : TupleDomain.all();

}

@albericgenius partitions.isEmpty() check is common in both if statements.
Please consider applying the suggested change.

Thanks and updated, I can see the improvement from your comments.

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

albericgenius · 2022-05-17T14:16:37Z

Thanks for sharing, it is really helpful
Updated the commit message header to smaller.

findinpath · 2022-05-18T05:14:50Z

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

+    public void testCreatePredicateWithEmptyPartition()
+    {
+        ImmutableList.Builder<HivePartition> partitions = ImmutableList.builder();
+        Domain domain = createPredicate(ImmutableList.of(TEST_COLUMN_HANDLE), partitions.build())


Use ImmutableList.of() instead of defining extra variable partitions for keeping the code smaller.

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

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

findinpath · 2022-05-18T08:25:35Z

Please add in the description:

Fixes #10398

findinpath · 2022-05-18T09:47:19Z

Please be more brief with implementation details in the commit message.
Think of explaining what you are doing and not every implementation detail how you did it.

albericgenius · 2022-05-18T12:52:48Z

Please add in the description:

Fixes #10398

Thanks for notes, I added :)

albericgenius · 2022-05-18T12:54:02Z

Yes, I have updated, English is not my first language, I try my best.
Thanks for your help and time.

albericgenius · 2022-05-19T05:47:31Z

@findepi and @findinpath
This is my first approved commit, I am not sure the process.
do I need the second approval here, or you can help me to merge into master branch?
Thanks
Alberic

findinpath · 2022-05-19T07:42:04Z

@albericgenius generally it may take a while (a few hours/ a few days) until a maintainer merges the commit.
In any case feel free to take a stab on another open issue in the meanwhile.

Good job!

@bitsondatadev we should probably document the PR process on https://github.com/trinodb/trino/blob/master/.github/DEVELOPMENT.md to avoid confusion.

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

findepi · 2022-05-19T08:00:55Z

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

@@ -2925,7 +2925,7 @@ public ConnectorTableHandle makeCompatiblePartitioning(ConnectorSession session,
    static TupleDomain<ColumnHandle> createPredicate(List<ColumnHandle> partitionColumns, List<HivePartition> partitions)
    {
        if (partitions.isEmpty()) {
-            return TupleDomain.none();
+            return partitionColumns.isEmpty() ? TupleDomain.none() : TupleDomain.all();


if (partitionColumns.isEmpty()) { // not a partitioned table checkArgument(partitions.size()==1 && UNPARTITIONED_ID.equals(getOnlyElement(partitions).getPartitionId()), "Unexpected partitions for a non-partitioned table: %s", partitions); return TupleDomain.all();

but then what remains would be (as it used to be)

if (partitions.isEmpty()) { return TupleDomain.none(); }

you return the opposite value. I don't understand why.

originally we return TupleDomain.none() when partitions.isEmpty()

but in the case of no data of the PartitionedTable, we will throw IllegalArgumentException in IoPlanPrinter.parseConstraints. because the constraint is none.

for no data of the PartitionedTable case, the partitions is empty, and partitionColumns is not empty

I could be wrong, please help to point out, I will update asap.

thanks for the explanation

but in the case of no data of the PartitionedTable, we will throw IllegalArgumentException in IoPlanPrinter.parseConstraints. because the constraint is none.

does it mean we should fix that method instead?

findepi · 2022-05-19T10:33:43Z

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

+                        Optional.empty(),
+                        estimate));
+
+        assertUpdate("DROP TABLE test_io_explain");


test_io_explain -> test_io_explain_with_empty_partitioned_table

using same table name as in the other test will make tests fail when run concurrently

#12349 (comment)

updated the table name.

IoPlanPrinter.parseConstraints will check the constraint is none or not. The logic is correct.

I think if a partitioned table do not have any data, we should return TupleDomain.all() as constraint. there is only one case that partitions is empty and partitionColumns is not because of no data.

Only partitionColumns and partitions are empty, we should return TupleDomain.none().

What is your thought?

I think if a partitioned table do not have any data, we should return TupleDomain.all() as constraint.

TupleDomain.none() is also correct ("no rows match the filter"), and may be more useful ("more correct"), as can allow pruning other parts of the query

This implement will not take effect "no rows match the filter". because there is no data(partitions is empty and partitionColumns is not empty), even we return TupleDomain.all(), there is no data match the filter, the result is same.

I agree with you, this is better to fix inside IoPlanPrinter.parseConstraints, but i still do not know how to get partitions informations in plan process. i will continue to think about it tomorrow. if you have free time, please help to give me some suggestion.

Thanks for your time
Alberic

even we return TupleDomain.all(), there is no data match the filter, the result is same.

io.trino.spi.connector.ConnectorMetadata#getTableProperties's io.trino.spi.connector.ConnectorTableProperties#predicate is used to inform the planner and allow deriving filters for other tables.

for example, a query

SELECT * FROM some_table JOIN empty_partitioned_table ON ...

should be reduced to SELECT .. WHERE false because the planner realizes empty_partitioned_table has no rows (TupleDomain.none()).

findepi · 2022-05-19T10:34:55Z

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

@@ -2925,7 +2925,7 @@ public ConnectorTableHandle makeCompatiblePartitioning(ConnectorSession session,
    static TupleDomain<ColumnHandle> createPredicate(List<ColumnHandle> partitionColumns, List<HivePartition> partitions)
    {
        if (partitions.isEmpty()) {
-            return TupleDomain.none();
+            return partitionColumns.isEmpty() ? TupleDomain.none() : TupleDomain.all();


thanks for the explanation

but in the case of no data of the PartitionedTable, we will throw IllegalArgumentException in IoPlanPrinter.parseConstraints. because the constraint is none.

does it mean we should fix that method instead?

findepi · 2022-05-23T12:41:08Z

core/trino-main/src/main/java/io/trino/sql/planner/planprinter/IoPlanPrinter.java

@@ -717,7 +716,9 @@ private EstimatedStatsAndCost getEstimatedStatsAndCost(TableScanNode node)

        private Set<ColumnConstraint> parseConstraints(TableHandle tableHandle, TupleDomain<ColumnHandle> constraint)
        {
-            checkArgument(!constraint.isNone());
+            if (constraint.isNone()) {
+                return ImmutableSet.of();


That's exactly what we would return if predicate is all, so it's probably not the right return value for the none case.

IoPlan.TableColumnInfo cannot currently represent a table with NONE constraint.

In general, such table should be eliminated from the query plan, this happens e.g. here

trino/core/trino-main/src/main/java/io/trino/sql/planner/iterative/rule/PushPredicateIntoTableScan.java

Lines 255 to 258 in 14b901d

TableProperties newTableProperties = plannerContext.getMetadata().getTableProperties(session, newTable);

Optional<TablePartitioning> newTablePartitioning = newTableProperties.getTablePartitioning();

if (newTableProperties.getPredicate().isNone()) {

return Optional.of(new ValuesNode(node.getId(), node.getOutputSymbols(), ImmutableList.of()));

We could fix the optimizer so that it happens as well in the SELECT * FROM empty_partitioned_table case and we probably should. However, this wouldn't eliminate the need to fix EXPLAIN (TYPE IO) from failing in such case, since it's generally a possible situation. That's why we deal with it also on page source level instead of failing there

trino/core/trino-main/src/main/java/io/trino/split/PageSourceManager.java

Line 63 in ad86637

return new EmptyPageSource();

So, back to our problem. We need to make IoPlan.TableColumnInfo be able to represent table scan without data, with none constraint.
I would suggest replacing Set<ColumnConstraint> columnConstraints field with

class Constraint { boolean isNone; Set<ColumnConstraint> columnConstraints; }

cc @martint @sopel39 @kokosing @losipiuk @JamesRTaylor

findepi · 2022-05-24T10:01:21Z

@albericgenius albericgenius force-pushed the mainline branch from 9a4b2d1 to 26ef697 17 hours ago

the diff is https://github.com/trinodb/trino/compare/9a4b2d177ee4d2ebd0c992c8385bc00fbab7c5ac..26ef697107add3b991a11775fe66764dfbce5837

@albericgenius please don't rebase, unless necessary.
it makes reviewing harder. Github should be able to show a diff that of changes that matter, but it's apparently not implemented yet.

albericgenius · 2022-09-15T11:25:53Z

@findepi Thanks for your help and time.
I try to follow your idea to add a Rule: RemoveEmptyTableScan. if the table is empty and partition is empty, i will replace the scan table node with a value node. Then the IoPlanPrinter issue is resolved.

I am not sure my implement way is match your idea or not? now it affect some JDBC test cases because of this new Rule.

If it is matched, i will continue to fix the JDBC test cases.
Otherwise still need your expertise and help to provide any comment. I will response as soon as possible.

findepi · 2022-09-16T14:08:59Z

@albericgenius
in #12349 (comment) i indicated we need to fix the IO plan printer

So, back to our problem. We need to make IoPlan.TableColumnInfo be able to represent table scan without data, with none constraint.
I would suggest replacing Set columnConstraints field with ...

However, i also see that we could choose NOT to fix the plan printer, and consider a plan with redundant TableScan left as "bogus" or "invalid". It's surely undesirable.

@martint thoughts?

martint · 2022-09-19T16:22:12Z

However, i also see that we could choose NOT to fix the plan printer, and consider a plan with redundant TableScan left as "bogus" or "invalid". It's surely undesirable.

A TableScan that produces no data is a perfectly valid plan, so that should be fixed in the plan printer. It might be undesirable from a performance perspective, but that's just an optimization.

findepi · 2022-09-19T20:05:04Z

core/trino-main/src/main/java/io/trino/sql/planner/planprinter/IoPlanPrinter.java

+    public static class Constraint
+    {
+        private final boolean isNone;
+


Suggested change

findepi · 2022-09-19T20:05:33Z

core/trino-main/src/main/java/io/trino/sql/planner/planprinter/IoPlanPrinter.java

+                @JsonProperty("columnConstraints") Set<ColumnConstraint> columnConstraints)
+        {
+            this.isNone = isNone;
+            this.columnConstraints = columnConstraints;


make a defensive copy to ensure that Constraint is immutable

Suggested change

this.columnConstraints = columnConstraints;

this.columnConstraints = ImmutableSet.copyIf(requireNonNull(columnConstraints, "columnConstraints is null"));

findepi · 2022-09-19T20:07:03Z

core/trino-main/src/main/java/io/trino/sql/planner/planprinter/IoPlanPrinter.java

@@ -697,13 +754,14 @@ private void addInputTableConstraints(TupleDomain<ColumnHandle> filterDomain, Ta
            TableMetadata tableMetadata = plannerContext.getMetadata().getTableMetadata(session, table);
            TupleDomain<ColumnHandle> predicateDomain = plannerContext.getMetadata().getTableProperties(session, table).getPredicate();
            EstimatedStatsAndCost estimatedStatsAndCost = getEstimatedStatsAndCost(tableScan);
+            boolean withoutData = estimatedStatsAndCost.getOutputRowCount() == 0d ? true : false;


The logic here should not depend on stats.
Stats are estimates, can be inaccurate and off.

findepi · 2022-09-19T20:08:52Z

core/trino-main/src/main/java/io/trino/sql/planner/planprinter/IoPlanPrinter.java

        {
-            checkArgument(!constraint.isNone());
+            checkArgument(!constraint.isNone() || withoutData);


i think we need to replace this check with an if

if (constraint.isNone()) { return new Constraint(true, ImmutableSet.of()); }

findepi · 2022-09-19T20:10:49Z

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

+                                        new IoPlanPrinter.Constraint(withoutData,
                                        ImmutableSet.of(


the indentation here is off and will change as soon as someone invokes reformatting

Suggested change

new IoPlanPrinter.Constraint(withoutData,

ImmutableSet.of(

new IoPlanPrinter.Constraint(withoutData, ImmutableSet.of(

findepi · 2022-09-19T20:11:32Z

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

@@ -1043,13 +1044,15 @@ public void testIoExplain()
        computeActual("CREATE TABLE test_io_explain WITH (partitioned_by = ARRAY['orderkey', 'processing']) AS SELECT custkey, orderkey, orderstatus = 'P' processing FROM orders WHERE orderkey < 3");

        EstimatedStatsAndCost estimate = new EstimatedStatsAndCost(2.0, 40.0, 40.0, 0.0, 0.0);
+        boolean withoutData = estimate.getOutputRowCount() == 0d ? true : false;


We now the expected result (constraint here shouldn't be none)
we don't need a variable and conditional initialization for that

findepi · 2022-09-19T20:11:48Z

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

@@ -1091,64 +1094,70 @@ public void testIoExplain()
        computeActual("CREATE TABLE test_io_explain WITH (partitioned_by = ARRAY['orderkey']) AS SELECT custkey, orderkey FROM orders WHERE orderkey < 200");

        estimate = new EstimatedStatsAndCost(55.0, 990.0, 990.0, 0.0, 0.0);
+        withoutData = estimate.getOutputRowCount() == 0d ? true : false;


as above -- replace with constant and inline

Currently, this can happen e.g. when a hive partitioned table is empty. Ideally, such a table scan should be eliminated from the plan, but plan printing should not rely on that, this would be just an optimization.

findepi · 2022-09-20T06:52:35Z

I applied some cosmetic changes (to the code & commit message) myself. Will merge once the build passes.

Thank you for your contribution!

albericgenius · 2022-09-20T08:39:18Z

I applied some cosmetic changes (to the code & commit message) myself. Will merge once the build passes.

Thank you for your contribution!

Thanks for your time and your coaching.

cla-bot bot added the cla-signed label May 12, 2022

github-actions bot added the tests:hive label May 12, 2022

findepi requested review from homar and findinpath May 12, 2022 19:36

findinpath reviewed May 13, 2022

View reviewed changes

albericgenius force-pushed the mainline branch from 5a45cde to 7b5b539 Compare May 17, 2022 14:13

findepi requested review from findinpath and homar and removed request for homar May 17, 2022 14:42

findinpath reviewed May 18, 2022

View reviewed changes

plugin/trino-hive/src/test/java/io/trino/plugin/hive/BaseHiveConnectorTest.java Outdated Show resolved Hide resolved

plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestHiveMetadata.java Outdated Show resolved Hide resolved

albericgenius force-pushed the mainline branch from 7b5b539 to 05f7c03 Compare May 18, 2022 08:22

findinpath approved these changes May 18, 2022

View reviewed changes

albericgenius force-pushed the mainline branch from 05f7c03 to 8a440be Compare May 18, 2022 09:37

albericgenius force-pushed the mainline branch from 8a440be to 67688f5 Compare May 18, 2022 12:52

albericgenius force-pushed the mainline branch from 67688f5 to 0184445 Compare May 18, 2022 13:02

findepi reviewed May 19, 2022

View reviewed changes

albericgenius force-pushed the mainline branch 4 times, most recently from e5d2a9f to e0a5f1b Compare May 19, 2022 10:24

findepi reviewed May 19, 2022

View reviewed changes

albericgenius force-pushed the mainline branch 2 times, most recently from 035afed to 9a4b2d1 Compare May 20, 2022 17:04

findinpath self-requested a review May 23, 2022 10:20

findinpath approved these changes May 23, 2022

View reviewed changes

findepi reviewed May 23, 2022

View reviewed changes

findinpath self-requested a review May 23, 2022 13:20

albericgenius force-pushed the mainline branch from 9a4b2d1 to 26ef697 Compare May 23, 2022 17:01

albericgenius force-pushed the mainline branch from 6fc32ec to 030f789 Compare September 15, 2022 11:14

albericgenius closed this Sep 15, 2022

albericgenius reopened this Sep 15, 2022

albericgenius force-pushed the mainline branch 3 times, most recently from d90cbf1 to 5254b78 Compare September 19, 2022 15:41

findepi requested changes Sep 19, 2022

View reviewed changes

albericgenius force-pushed the mainline branch from 5254b78 to dd8ac8c Compare September 20, 2022 06:35

Fix IO EXPLAIN when table scan has none constraint

d8f33d4

Currently, this can happen e.g. when a hive partitioned table is empty. Ideally, such a table scan should be eliminated from the plan, but plan printing should not rely on that, this would be just an optimization.

findepi force-pushed the mainline branch from dd8ac8c to d8f33d4 Compare September 20, 2022 06:51

findepi approved these changes Sep 20, 2022

View reviewed changes

findepi merged commit 6a4f1ca into trinodb:master Sep 20, 2022

github-actions bot added this to the 397 milestone Sep 20, 2022

This was referenced Sep 20, 2022

Explain (TYPE IO) will throw Exception when table is hive partition table which is empty #10398

Closed

Release notes for 397 #14154

Closed

colebow mentioned this pull request Sep 20, 2022

Add Trino 397 release notes #14194

Merged

albericgenius deleted the mainline branch September 28, 2022 03:37

	TableProperties newTableProperties = plannerContext.getMetadata().getTableProperties(session, newTable);
	Optional<TablePartitioning> newTablePartitioning = newTableProperties.getTablePartitioning();
	if (newTableProperties.getPredicate().isNone()) {
	return Optional.of(new ValuesNode(node.getId(), node.getOutputSymbols(), ImmutableList.of()));

	this.columnConstraints = columnConstraints;
	this.columnConstraints = ImmutableSet.copyIf(requireNonNull(columnConstraints, "columnConstraints is null"));

	new IoPlanPrinter.Constraint(withoutData,
	ImmutableSet.of(
	new IoPlanPrinter.Constraint(withoutData, ImmutableSet.of(

Fix the explain (TYPE IO) Exception when table is hive partition tabl… #12349

Fix the explain (TYPE IO) Exception when table is hive partition tabl… #12349

Conversation

albericgenius commented May 12, 2022

Description

Related issues, pull requests, and links

Documentation

Release notes

findinpath commented May 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albericgenius May 18, 2022 • edited Loading

Choose a reason for hiding this comment

albericgenius commented May 17, 2022

findinpath May 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

findinpath commented May 18, 2022

findinpath commented May 18, 2022

albericgenius commented May 18, 2022

albericgenius commented May 18, 2022

albericgenius commented May 19, 2022

findinpath commented May 19, 2022

Choose a reason for hiding this comment

albericgenius May 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albericgenius May 19, 2022 • edited Loading

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

findepi commented May 24, 2022

albericgenius commented Sep 15, 2022 • edited Loading

findepi commented Sep 16, 2022

martint commented Sep 19, 2022

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

findepi commented Sep 20, 2022

albericgenius commented Sep 20, 2022

albericgenius May 18, 2022 •

edited

Loading

findinpath May 18, 2022 •

edited

Loading

albericgenius May 19, 2022 •

edited

Loading

albericgenius May 19, 2022 •

edited

Loading

albericgenius commented Sep 15, 2022 •

edited

Loading