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

Avoid Marker intermediate representation in DomainCoercer #6976

Merged
merged 2 commits into from Feb 22, 2021

Conversation

findepi
Copy link
Member

@findepi findepi commented Feb 20, 2021

No description provided.

This helps use this method correctly, when it may not be known upfront
whether operands have a non-empty intersection.

It is a consciously a breaking change, as the method is rarely used (has
0 usages in the project), so accommodating to the change is expected to
be easier than maintaining transition period.
@cla-bot cla-bot bot added the cla-signed label Feb 20, 2021
@findepi findepi added the maintenance Project maintenance task label Feb 20, 2021
@findepi findepi merged commit f15ea75 into trinodb:master Feb 22, 2021
@findepi findepi deleted the findepi/demark-domain=coercer branch February 22, 2021 11:01
@findepi
Copy link
Member Author

findepi commented Feb 22, 2021

thanks for review.
What bothers me still is whether we need the logic in DomainCoercer at all.
This was already touched upon here #5461 (comment)

@sopel39
Copy link
Member

sopel39 commented Feb 22, 2021

This is because saturated floor/ceil casts are more generic (e.g could be used to reverse cast from date to timestamp). I believe this was discussed once with @martin on slack.

@findepi
Copy link
Member Author

findepi commented Feb 22, 2021

I actually am under impression that floor casts are not more generic. For example, we do not have implementation for these casts from bigint to double, or decimal. While the latter can be added, the former cannot (afaiu). The added advantage of not expressing this with a cast, is that the logic can be dependent on particular values.

I am curious to learn more, but indeed a closed PR is not the right place for this discussion.

@findepi findepi mentioned this pull request Mar 2, 2021
10 tasks
@martint martint added this to the 353 milestone Mar 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed maintenance Project maintenance task
Development

Successfully merging this pull request may close these issues.

None yet

4 participants