Return int32 for integer type date part #13466
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: jayzhan211 <[email protected]>
jayzhan211
changed the title
Signed-off-by: jayzhan211 <[email protected]>
findepi
approved these changes
Nov 18, 2024
| | "qtr" | ||
| | "quarter" | ||
| | "doy" | ||
| | "dow" |
There was a problem hiding this comment.
second, millisecond, micro, nanos too?
There was a problem hiding this comment.
let r: Float64Array = secs
.iter()
.zip(subsecs)
.map(|(secs, subsecs)| {
secs.map(|secs| {
let subsecs = subsecs.unwrap_or(0);
(secs as f64 + ((subsecs % 1_000_000_000) as f64 / 1_000_000_000_f64))
* sf
})
})
.collect();Probably we still need f64 for seconds
There was a problem hiding this comment.
i didn't know extract(second .. returns seconds with fraction. In PostgreSQL it does so, in e.g. Trino and Snowflake it does not
postgres=# select extract(second from now());
extract
-----------
29.009019
trino> select extract(second from now());
_col0
-------
46
so yes, keep seconds with float for now
There was a problem hiding this comment.
extract for seconds is float correct, returning the fractions. however its not the case for DuckDB.
There was a problem hiding this comment.
@jayzhan211 what about millisecond, micro, nano?
| // Try to remove quote if exist, if the quote is invalid, return original string and let the downstream function handle the error | ||
| fn part_normalization(part: &str) -> &str { | ||
| part.strip_prefix(|c| c == '\'' || c == '\"') | ||
| .and_then(|s| s.strip_suffix(|c| c == '\'' || c == '\"')) |
There was a problem hiding this comment.
why would we want to support unmatched quotes like '..."?
the date_part argument shouldn't contain any quotes, why would we want to support quotes at all?
i see this is pre-existing, so okay to keep, but still wonder why we're doing this, doesn't feel right
| SELECT extract(year from arrow_cast('10 years', 'Interval(YearMonth)')) | ||
| ---- | ||
| 10 | ||
|
|
||
| query R | ||
| query I |
There was a problem hiding this comment.
Should we have explicit tests inspecting arrow_typeof(extract(....))?
There was a problem hiding this comment.
Since I is for integer and we don't need to make sure it is exactly i32 or i64 so I think that is fine. If we want to differentiate type like utf8 or utf8View then we need arrow_typeof
There was a problem hiding this comment.
Since
Iis for integer
yes i know
need to make sure it is exactly i32 or i64
do we? do we not?
i think it may make difference, since it's 2x fewer bytes to keep in memory
up to you
|
Signed-off-by: jayzhan211 <[email protected]>
Dandandan
reviewed
Nov 19, 2024
Co-authored-by: Daniël Heres <[email protected]>
Signed-off-by: jayzhan211 <[email protected]>
Signed-off-by: jayzhan211 <[email protected]>
comphead
reviewed
Nov 20, 2024
| return internal_err!("unit {unit:?} not supported"); | ||
| } | ||
|
|
||
| let conversion_factor = match unit { |
There was a problem hiding this comment.
we should probably factor out the common part for this conversion in future. its too many hardcoded conversions between second fraction and corresponding long value in the project.
comphead
approved these changes
Nov 20, 2024
Signed-off-by: Jay Zhan <[email protected]>
|
Thanks @comphead @findepi @Dandandan |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Close #13482
Part of #13449
Rationale for this change
Date part such as year, month, day are integer type, we can return i32 instead of casting it to f64
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?