Skip to content

Add timestamp ns literal#3298

Open
rambleraptor wants to merge 1 commit intoapache:mainfrom
rambleraptor:timestampns_literals
Open

Add timestamp ns literal#3298
rambleraptor wants to merge 1 commit intoapache:mainfrom
rambleraptor:timestampns_literals

Conversation

@rambleraptor
Copy link
Copy Markdown
Contributor

Rationale for this change

Ran into this with the type promotion rabbit hole I've found myself in.

Turns out we don't have literal types for timestamp_ns. This is definitely a missing part of being able to use timestamp_ns types.

My goal here was to match the microsecond usage as much as possible.

(There's some weirdness for converting these to strings, since Python's string format doesn't actually handle nano-seconds for us. What fun!)

Are these changes tested?

Added some tests.

Are there any user-facing changes?

  • Adds literals for timestamp_ns

@rambleraptor
Copy link
Copy Markdown
Contributor Author

@geruh @kevinjqliu found this while working on equality deletes + type promotion stuff. Should be pretty straight forward. ptal, thanks for your time!

Comment on lines +342 to +348
@to.register(TimestampNanoType)
def _(self, _: TimestampNanoType) -> Literal[int]:
return TimestampNanoLiteral(self.value)

@to.register(TimestamptzNanoType)
def _(self, _: TimestamptzNanoType) -> Literal[int]:
return TimestampNanoLiteral(self.value)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Unfortunally, this has been shown to be controversial in the Iceberg-Java implementation: apache/iceberg#11775 (comment)

If you think about it in SQL: SELECT * FROM tbl WHERE dt > 1777883174. This assumes that the int is micros. However, when you would widen the field dt by converting it from micros to nanos, then the query would return a different result. When we would bind the LongLiteral to TimestampNanoLiteral we implicitly assume that it is a nanos after the widening of the column.

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.

2 participants