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

feat: add extra option for on domain errors in log functions #536

Merged
merged 4 commits into from
Feb 28, 2024

Conversation

richtia
Copy link
Member

@richtia richtia commented Aug 4, 2023

BREAKING CHANGE: Adding a NULL option to the on_domain_errors.

SQLite returns null for some inputs such as negative infinity

EpsilonPrime
EpsilonPrime previously approved these changes Sep 27, 2023
Copy link
Member

@EpsilonPrime EpsilonPrime left a comment

Choose a reason for hiding this comment

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

This appears to be a breaking change as a new producer could send this new option to an old consumer which wouldn't know what to do with it.

Otherwise I'm good to go with this PR.

@jacques-n
Copy link
Contributor

Should we use NULL instead of NONE if the behavior is to return a NULL if a domain error occurs?

EpsilonPrime
EpsilonPrime previously approved these changes Nov 22, 2023
@richtia
Copy link
Member Author

richtia commented Feb 23, 2024

Should we use NULL instead of NONE if the behavior is to return a NULL if a domain error occurs?

Reverted back to using NONE. NULL seems to be a reserved word, so it was causing the yaml extension validation to fail

@westonpace
Copy link
Member

I do like NULL. You are right it is a reserved word in YAML but you should be able to make it work with quotes: [ NAN, "NULL", ERROR ]

@richtia
Copy link
Member Author

richtia commented Feb 28, 2024

I do like NULL. You are right it is a reserved word in YAML but you should be able to make it work with quotes: [ NAN, "NULL", ERROR ]

This is what I originally wanted to do as well...but putting NULL around quotes seemed a little different than the convention we were following. I'm okay with doing this moving forward so long as everyone else is too. Just made the update

@richtia richtia requested a review from EpsilonPrime February 28, 2024 17:24
@richtia richtia merged commit cbec079 into substrait-io:main Feb 28, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants