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

fix: allow interval cast-related functions to accept only literals instead of evaluations #1238

Merged

Conversation

ever0de
Copy link
Member

@ever0de ever0de commented Jun 5, 2023

  • remote async [Value::try_cast_from_literal, Interval::parse]
  • remove DataType::Interval cast to Value
  • remove Value::try_into_interval
  • Rename From<&Interval> for String to to_sql_str

@ever0de ever0de requested review from panarch and devgony June 5, 2023 10:57
@ever0de ever0de self-assigned this Jun 5, 2023
@ever0de ever0de force-pushed the fix/do-not-allow-cast-of-value-to-interval branch from f7147ae to eb96e7f Compare June 5, 2023 11:34
@coveralls
Copy link

coveralls commented Jun 5, 2023

Pull Request Test Coverage Report for Build 5177060014

  • 47 of 48 (97.92%) changed or added relevant lines in 12 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.08%) to 98.907%

Changes Missing Coverage Covered Lines Changed/Added Lines %
core/src/data/interval/string.rs 11 12 91.67%
Files with Coverage Reduction New Missed Lines %
core/src/data/value/mod.rs 1 99.62%
Totals Coverage Status
Change from base Build 5177030840: 0.08%
Covered Lines: 45229
Relevant Lines: 45729

💛 - Coveralls

@ever0de ever0de force-pushed the fix/do-not-allow-cast-of-value-to-interval branch 4 times, most recently from 9abbbcf to 196b74b Compare June 5, 2023 12:01
@ever0de ever0de changed the title fix: don't allow cast of Value to Interval, fix: allow interval cast-related functions to accept only literals instead of evaluations Jun 5, 2023
@@ -173,7 +172,7 @@ impl JsonStorage {

let value = match value.get_type() {
Some(data_type) if data_type != column_def.data_type => {
block_on(value.cast(&column_def.data_type))?
value.cast(&column_def.data_type)?
Copy link
Member

Choose a reason for hiding this comment

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

here was the only case which async_io was used in json-storage.
may be we can remove async_io in json storage Cargo.toml

.await
.and_then(Value::try_from)
.map(String::from)?;
let literal = match &*expr {
Copy link
Member

Choose a reason for hiding this comment

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

rather than &*expr, expr.as_ref() would be simple.

- remote async [Value::try_cast_from_literal, Interval::parse]
- remove `DataType::Interval` cast to Value
- remove `Value::try_into_interval`
- Rename `From<&Interval> for String` to `to_sql_str`
@ever0de ever0de force-pushed the fix/do-not-allow-cast-of-value-to-interval branch from 60e8eb3 to 6709f06 Compare June 5, 2023 12:23
@panarch panarch self-requested a review June 5, 2023 12:27
@panarch panarch added the improvement Improvements for existing features label Jun 5, 2023
Copy link
Member

@panarch panarch left a comment

Choose a reason for hiding this comment

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

Awesome improvement! Thanks a lot 👍 👍

@panarch panarch merged commit 81a4d9e into gluesql:main Jun 5, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improvements for existing features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants