Skip to content

feat: Add support for specifying Timestamp ValueStr output layout#510

Merged
zeroshade merged 4 commits into
apache:mainfrom
cloudquery:feat/expose_timestamp_string_format
Sep 18, 2025
Merged

feat: Add support for specifying Timestamp ValueStr output layout#510
zeroshade merged 4 commits into
apache:mainfrom
cloudquery:feat/expose_timestamp_string_format

Conversation

@erezrokah
Copy link
Copy Markdown
Contributor

Rationale for this change

See discussion in #450

What changes are included in this PR?

Allow setting the formatting of timestamps when using ValueStr (or JSON marshalling)

Are these changes tested?

Yes - re-added the original test that was modified in #450

Are there any user-facing changes?

Yes - users will now be able to revert back to the old format before #450, to avoid unintentional breakage


func (t TimestampMicros) MarshalJSON() ([]byte, error) {
ts := time.Unix(0, int64(t)*int64(time.Microsecond)).UTC().Format(time.RFC3339Nano)
// arrow record marshaller trims trailing zero digits from timestamp so we do the same
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This comment seems no longer relevant

Copy link
Copy Markdown

@efd6 efd6 left a comment

Choose a reason for hiding this comment

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

this seems reasonable

Comment thread arrow/array/timestamp.go Outdated
return NewTimestampDataWithFormat(data, time.RFC3339Nano)
}

func NewTimestampDataWithFormat(data arrow.ArrayData, format string) *Timestamp {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
func NewTimestampDataWithFormat(data arrow.ArrayData, format string) *Timestamp {
func NewTimestampDataWithLayout(data arrow.ArrayData, layout string) *Timestamp {

reflecting the terminology used in stdlib/time (s/format/layout/g throughout)

Also probably want godoc for this.

@erezrokah erezrokah changed the title feat: Add support for specifying Timestamp valueStr output format feat: Add support for specifying Timestamp valueStr output layout Sep 17, 2025
@erezrokah erezrokah marked this pull request as ready for review September 17, 2025 18:14
@erezrokah erezrokah requested a review from efd6 September 17, 2025 18:17
@erezrokah erezrokah changed the title feat: Add support for specifying Timestamp valueStr output layout feat: Add support for specifying Timestamp ValueStr output layout Sep 17, 2025
Copy link
Copy Markdown
Member

@zeroshade zeroshade left a comment

Choose a reason for hiding this comment

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

Just some nitpicks

Comment thread arrow/array/timestamp.go Outdated
Comment thread arrow/array/timestamp.go Outdated
@erezrokah erezrokah requested a review from zeroshade September 18, 2025 11:41
Copy link
Copy Markdown
Member

@zeroshade zeroshade left a comment

Choose a reason for hiding this comment

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

Looks good to me, just one nitpick: can we add a JSON test which verifies the JSON marshalling output respects the layout

@erezrokah
Copy link
Copy Markdown
Contributor Author

Looks good to me, just one nitpick: can we add a JSON test which verifies the JSON marshalling output respects the layout

Added a test in 36fd689, and discovered I had a redundant method func (b *TimestampBuilder) NewTimestampArrayWithFormat(format string) (a *Timestamp) which I removed

@zeroshade zeroshade merged commit d241a6e into apache:main Sep 18, 2025
15 of 16 checks passed
@erezrokah
Copy link
Copy Markdown
Contributor Author

Thank you @zeroshade and @efd6 for the swift review process, appreciate it

@erezrokah
Copy link
Copy Markdown
Contributor Author

Hi @zeroshade any chance of doing a release with this feature soon?

kodiakhq Bot pushed a commit to cloudquery/plugin-sdk that referenced this pull request Dec 16, 2025
#2367)



Instead of #2306 that also updates the Arrow version to v18.5.0 with apache/arrow-go#510.

This PR has lint fixes and test fixes using apache/arrow-go#510

---
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.

3 participants