Skip to content

Handle coalesce partition handle when bucket count does not match but compatible#14267

Merged
rongrong merged 2 commits intoprestodb:masterfrom
rongrong:relax-check
Mar 24, 2020
Merged

Handle coalesce partition handle when bucket count does not match but compatible#14267
rongrong merged 2 commits intoprestodb:masterfrom
rongrong:relax-check

Conversation

@rongrong
Copy link
Contributor

@rongrong rongrong commented Mar 19, 2020

Should we have release note for this? It's technically a fix. Also do we have release note for default session value changes?

== RELEASE NOTES ==
General Changes
* Fix query failures with incompatible partition handle when session property ``partial_merge_pushdown_strategy`` is set to ``PUSH_THROUGH_LOW_MEMORY_OPERATORS`` and ``optimize_full_outer_join_with_coalesce`` is set to ``true`` and query has mismatched partition and uses ``FULL OUTER JOIN`` with ``COALESCE``.

@rongrong rongrong requested a review from wenleix March 19, 2020 01:33
@rongrong
Copy link
Contributor Author

cc @caithagoras

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also update the error message?

@caithagoras
Copy link
Contributor

caithagoras commented Mar 19, 2020

Consider squashing the commits into one: You can add descriptions about default value changes into the commit message of the 3rd commit.

@caithagoras
Copy link
Contributor

caithagoras commented Mar 19, 2020

We should have release notes for the fix. Something like

General Changes
* Fix query failure for ``CROSS JOIN`` with ``COALESCE`` when bucket counts are
  compatible but mismatched.

@caithagoras
Copy link
Contributor

Thanks for the fix! lgtm, will let @wenleix take a look.

Copy link
Contributor

@wenleix wenleix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM % minor comment.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about:

    this.handle.equals(other.handle) || metadata.isRefinedPartitioningOver(session, this, other) || metadata.isRefinedPartitioningOver(session, other, this)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about

   && metadata.isRefinedPartitioningOver(session, probeProperties.getNodePartitioning().get().getHandle(), buildProperties.getNodePartitioning().get().getHandle()).isPresent()) || metadata.isRefinedPartitioningOver()...

You might want to save probeProperties.getNodePartitioning().get().getHandle() and buildProperties.getNodePartitioning().get().getHandle()).isPresent() as local variable.

@wenleix
Copy link
Contributor

wenleix commented Mar 19, 2020

optimize_mismatched_bucket_count cannot be default to true, this is because engine cannot reason whether the "compatible bucket" will result in more memory usage or not, for example:

          AGGR
            |
          JOIN
         /     \
 TS(2048)    TS(128)

Now it will be executed as if there are 128 buckets, and cause increase in local memory usage. This is the motivation to deprecate optimize_mismatched_bucket_count and introduce refined partitioning in #12611, so engine can reason why the partitioning is refined or coarsened .

Copy link
Contributor

@wenleix wenleix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@rongrong rongrong merged commit 68e4367 into prestodb:master Mar 24, 2020
@caithagoras caithagoras mentioned this pull request Mar 29, 2020
9 tasks
@caithagoras caithagoras mentioned this pull request May 4, 2020
34 tasks
@rongrong rongrong deleted the relax-check branch November 23, 2021 22:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants