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

Translate CAST to connector expression #11551

Merged
merged 2 commits into from
Apr 8, 2022

Conversation

wendigo
Copy link
Contributor

@wendigo wendigo commented Mar 17, 2022

Extracted first commit from #11522

@wendigo wendigo requested a review from findepi March 17, 2022 18:43
@wendigo
Copy link
Contributor Author

wendigo commented Mar 21, 2022

@findepi PTAL

@wendigo wendigo force-pushed the serafin/translate-cast-only branch from 8a26e3a to 4895ebd Compare March 21, 2022 10:36
@cla-bot cla-bot bot added the cla-signed label Mar 21, 2022
@wendigo
Copy link
Contributor Author

wendigo commented Mar 21, 2022

Rebased to include #11567

@findepi
Copy link
Member

findepi commented Mar 21, 2022

CI failure looks related.

@wendigo wendigo force-pushed the serafin/translate-cast-only branch from 4895ebd to 9b52315 Compare March 21, 2022 17:12
@wendigo
Copy link
Contributor Author

wendigo commented Mar 21, 2022

@findepi fixed

@wendigo wendigo force-pushed the serafin/translate-cast-only branch from 9b52315 to d8943b3 Compare March 22, 2022 12:22
@findepi
Copy link
Member

findepi commented Mar 25, 2022

CI: #11275

@findepi
Copy link
Member

findepi commented Mar 25, 2022

CAST has the property that the target type is known, so it probably shouldn't be ConnectorExpression.
If it is, there is no way to validate it's correctness.

I think we should model the CAST as a Cast class, new type of ConnectorExpression.
I will talk this with Martin

cc @martint @hashhar @losipiuk @assaf2

@wendigo wendigo force-pushed the serafin/translate-cast-only branch from d8943b3 to 5663f25 Compare March 28, 2022 08:48
@wendigo
Copy link
Contributor Author

wendigo commented Mar 28, 2022

@findepi ptal

@findepi
Copy link
Member

findepi commented Mar 31, 2022

I discussed this with @martint yesterday, and the conclusion was to model the CAST as a 1-arg $cast function.

The concerns were that it would prevent type inference if it is needed in the future, but we believe it won't be needed nor desired anyway. Also, our ConnectorExpression "language" doesn't have implicit coercions, and we don't want to change this.

@wendigo wendigo force-pushed the serafin/translate-cast-only branch 2 times, most recently from 4d028ea to 150c3d0 Compare April 7, 2022 08:59
@wendigo wendigo requested a review from findepi April 7, 2022 09:00
@findepi findepi force-pushed the serafin/translate-cast-only branch from 150c3d0 to 2b5c7a5 Compare April 7, 2022 10:51
@findepi findepi force-pushed the serafin/translate-cast-only branch from 2b5c7a5 to 71d3fa9 Compare April 7, 2022 10:52
@findepi
Copy link
Member

findepi commented Apr 7, 2022

there are some failures

@wendigo
Copy link
Contributor Author

wendigo commented Apr 7, 2022

2022-04-07T13:29:57.5906909Z org.testng.internal.thread.ThreadTimeoutException: Method io.trino.execution.resourcegroups.db.TestQueuesDb.testSelectorPriority() didn't finish within the time-out 60000

@wendigo
Copy link
Contributor Author

wendigo commented Apr 7, 2022

#11850

@findepi findepi merged commit b7cd83c into trinodb:master Apr 8, 2022
@findepi findepi added the no-release-notes This pull request does not require release notes entry label Apr 8, 2022
@github-actions github-actions bot added this to the 377 milestone Apr 8, 2022
@wendigo wendigo deleted the serafin/translate-cast-only branch April 8, 2022 10:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed no-release-notes This pull request does not require release notes entry
Development

Successfully merging this pull request may close these issues.

3 participants