Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove instance checks in PositionAppenders #13268

Merged
merged 2 commits into from
Jul 21, 2022

Conversation

sopel39
Copy link
Member

@sopel39 sopel39 commented Jul 20, 2022

Current Type contract does not enforce
particular block type for data.
Hence PositionAppenders should work with
generic Block type.

Description

Is this change a fix, improvement, new feature, refactoring, or other?

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

How would you describe this change to a non-technical end user or system administrator?

Related issues, pull requests, and links

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:

# General
* Fix query failures when UUID type is used. ({issue}`issuenumber`)

Fixes: #13265

sopel39 added 2 commits July 20, 2022 21:54
Current Type contract does not enforce
particular block type for data.
Hence PositionAppenders should work with
generic Block type.
@sopel39 sopel39 requested a review from martint July 20, 2022 19:57
@cla-bot cla-bot bot added the cla-signed label Jul 20, 2022
@sopel39
Copy link
Member Author

sopel39 commented Jul 20, 2022

cc @lukasz-stec

Copy link
Member

@lukasz-stec lukasz-stec left a comment

Choose a reason for hiding this comment

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

I would drop the TODOs, multiple Block types for a given Type are problematic for the entire engine, not only here.
Other than that, lgtm

@sopel39 sopel39 merged commit 94a87da into trinodb:master Jul 21, 2022
@sopel39 sopel39 mentioned this pull request Jul 21, 2022
@findepi findepi deleted the ks/remove_checks_appenders branch July 21, 2022 06:54
@findepi
Copy link
Member

findepi commented Jul 21, 2022

Should this have a regression test?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Failure in PartitionedOutputOperator due to incorrect expectation
4 participants