Skip to content

Conversation

@seddonm1
Copy link
Contributor

Hi @andygrove

This change adds the TRY_CAST alongside CAST so that the choice on how to handle data casting errors can be differentiated.

See: https://docs.microsoft.com/en-us/sql/t-sql/functions/try-cast-transact-sql?view=sql-server-ver15

Also fixed a clippy error that must have been added recently.

@coveralls
Copy link

coveralls commented Mar 17, 2021

Pull Request Test Coverage Report for Build 662686711

  • 24 of 25 (96.0%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.03%) to 90.046%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/parser.rs 15 16 93.75%
Totals Coverage Status
Change from base Build 552534083: 0.03%
Covered Lines: 5428
Relevant Lines: 6028

💛 - Coveralls

Copy link
Member

@andygrove andygrove left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @seddonm1

@seddonm1
Copy link
Contributor Author

How does the release process work for this library?

@andygrove
Copy link
Member

How does the release process work for this library?

I'm embarrassed to say that I don't really know anymore. @Dandandan released a version more recently than me so maybe he or @maxcountryman can help with that.

@seddonm1
Copy link
Contributor Author

@andygrove I think we can let you off the hook for not knowing given the work you have done building DataFusion and Ballista 😁

@Dandandan
Copy link
Contributor

@seddonm1 @andygrove

thanks a lot, I will have a look at this PR later.
The release process is documented here: https://github.com/ballista-compute/sqlparser-rs/blob/main/docs/releasing.md
Basically fully automated after doing a version bump.

@seddonm1
Copy link
Contributor Author

Thanks @Dandandan

@maxcountryman
Copy link
Contributor

Happy to help with a release. As @Dandandan mentioned, it should be automated once we've pushed a tag.

@seddonm1
Copy link
Contributor Author

@Dandandan are you able to review and hopefully release? This will allow us to add the CastOptions apache/arrow#9682 to DataFusion - hopefully before the next major release.

},
/// TRY_CAST an expression to a different data type e.g. `TRY_CAST(foo AS VARCHAR(123))`
// this differs from CAST in the choice of how to implement invalid conversions
TryCast {
Copy link
Contributor

Choose a reason for hiding this comment

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

I am wondering, why the choice TryCast / try_cast?

I know bigquery uses SAFE_CAST https://cloud.google.com/bigquery/docs/reference/standard-sql/functions-and-operators#safe_casting

It would make sense to support both maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could easily change this to add SAFE_CAST too. There was some rationale discussed here: apache/arrow#9682 (comment)

basically it was just that I think SQL Server got there first with TRY_CAST

@Dandandan
Copy link
Contributor

I think this looks good @seddonm1 ! I think we are good to go, do you want to address the SAFE_CAST question?

@Dandandan Dandandan merged commit e6e37b4 into apache:main Mar 21, 2021
@Dandandan
Copy link
Contributor

@seddonm1 will be released as 0.9.0 , thanks!

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.

5 participants