You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "zeroshade (via GitHub)" <gi...@apache.org> on 2025/07/25 15:27:26 UTC
[PR] fix(arrow/array): update timestamp json format [arrow-go]
zeroshade opened a new pull request, #450:
URL: https://github.com/apache/arrow-go/pull/450
### Rationale for this change
resolves #445
### What changes are included in this PR?
Use `time.RFC3339Nano` for the format to output the time value
### Are these changes tested?
Yes
### Are there any user-facing changes?
Changes the format of timestamp strings for `ValueStr` and marshalling to JSON to match JSON and Go conventions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(arrow/array): update timestamp json format [arrow-go]
Posted by "erezrokah (via GitHub)" <gi...@apache.org>.
erezrokah commented on PR #450:
URL: https://github.com/apache/arrow-go/pull/450#issuecomment-3274091046
> @erezrokah What would be the default? At this stage any choice would be a breaking change.
Technically what you would usually do is deprecate v18.4.1 (maybe also make it a pre-release so automated tools like renovate/dependabot don't pick it up), then release v18.4.2 with the old behavior, then do v18.5.0 to control the format (since it's a new feature).
But I get that this could create more issues.
We can keep the new format as the default if that sounds good to everyone
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(arrow/array): update timestamp json format [arrow-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on PR #450:
URL: https://github.com/apache/arrow-go/pull/450#issuecomment-3272393793
I'd be open to adding a configuration to control the format, but given the current release the default should be the current format used in the most recent release.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(arrow/array): update timestamp json format [arrow-go]
Posted by "erezrokah (via GitHub)" <gi...@apache.org>.
erezrokah commented on PR #450:
URL: https://github.com/apache/arrow-go/pull/450#issuecomment-3269987986
@zeroshade Are you open to adding a configuration to control the format? I'm happy to submit a PR for it.
Below some places I found that this change can be breaking (used [this search](https://github.com/search?q=%22.ValueStr%28i%29%22+language%3AGo&type=code&l=Go))
- https://github.com/openGemini/openGemini/blob/9742d18bd6c9910ee48b6e03bb54e2d0797da05e/engine/executor/chunk_arrow_transform.go#L477
- https://github.com/cloudquery/cloudquery/blob/b0d23841baad2b873e8ab90cc0aeba31713c462a/plugins/destination/sqlite/client/transformer.go#L45
- https://github.com/cloudquery/filetypes/blob/5a55b3d01859f13f01c8ae6ceaabb1200e5b9b05/json/write.go#L44
- https://github.com/cloudquery/filetypes/blob/5a55b3d01859f13f01c8ae6ceaabb1200e5b9b05/csv/write.go#L109
- https://github.com/milvus-io/milvus/blob/41820a09d011cfdbe7336240beb8f108bfc129e0/internal/storage/data_codec.go#L951
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(arrow/array): update timestamp json format [arrow-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade merged PR #450:
URL: https://github.com/apache/arrow-go/pull/450
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(arrow/array): update timestamp json format [arrow-go]
Posted by "efd6 (via GitHub)" <gi...@apache.org>.
efd6 commented on PR #450:
URL: https://github.com/apache/arrow-go/pull/450#issuecomment-3276646675
Thanks, @zeroshade. Yes, that works for me. We have just merged a change that depends on the new behaviour and would be sad if that went away via a retraction (even is this is really just an inconvenience to update a to-be-created knob).
It's unfortunate that the old behaviour sits right at the boundary of bug/feature. If it were more clearly a bug (I _do_ believe that it sits on that side of the line), there'd be no argument here that a breaking change would be justified.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(arrow/array): update timestamp json format [arrow-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on PR #450:
URL: https://github.com/apache/arrow-go/pull/450#issuecomment-3267445832
Personally I consider this a bug fix, that the format *should* have been this way originally and would only cause an issue if the consumer of the JSON was using a very strict parsing which didn't follow RFC3339.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(arrow/array): update timestamp json format [arrow-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on PR #450:
URL: https://github.com/apache/arrow-go/pull/450#issuecomment-3275782254
> Technically what you would usually do is deprecate v18.4.1 (maybe also make it a pre-release so automated tools like renovate/dependabot don't pick it up), then release v18.4.2 with the old behavior, then do v18.5.0 to control the format (since it's a new feature).
So far no one has filed an issue or otherwise complained about the format change in v18.4.1. As a result, my preference is to keep the new format as the default unless it comes up as an issue.
That work for you @erezrokah ? @efd6 ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(arrow/array): update timestamp json format [arrow-go]
Posted by "erezrokah (via GitHub)" <gi...@apache.org>.
erezrokah commented on PR #450:
URL: https://github.com/apache/arrow-go/pull/450#issuecomment-3298273684
Opened https://github.com/apache/arrow-go/pull/510 as draft for feedback, tried to do it in a way that doesn't breaking exist code
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(arrow/array): update timestamp json format [arrow-go]
Posted by "efd6 (via GitHub)" <gi...@apache.org>.
efd6 commented on PR #450:
URL: https://github.com/apache/arrow-go/pull/450#issuecomment-3124715396
Yeah, this change is essentially what I was hoping for. Thanks.
Thanks @amoeba for the test suggestion; this will prevent future skew away from stdlib JSON behaviour.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(arrow/array): update timestamp json format [arrow-go]
Posted by "erezrokah (via GitHub)" <gi...@apache.org>.
erezrokah commented on PR #450:
URL: https://github.com/apache/arrow-go/pull/450#issuecomment-3267684588
@zeroshade completely agree this is bug fix and this is how the format should have been implemented.
However, if someone is using this module as a middleware to generate JSONs for others to consume, they can't control the parsing down the line and it's impossible for them to know how this will impact their consumers.
I'm not sure that SemVer differentiates between a "big" breaking change and a "small" breaking change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(arrow/array): update timestamp json format [arrow-go]
Posted by "erezrokah (via GitHub)" <gi...@apache.org>.
erezrokah commented on PR #450:
URL: https://github.com/apache/arrow-go/pull/450#issuecomment-3267093917
Hi all 👋 Thanks for the fix, the new format makes sense. Shouldn't this have been marked as a breaking change? Consumers of the JSON might expect the old format. Maybe put it behind an option so it's opt-in?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(arrow/array): update timestamp json format [arrow-go]
Posted by "erezrokah (via GitHub)" <gi...@apache.org>.
erezrokah commented on PR #450:
URL: https://github.com/apache/arrow-go/pull/450#issuecomment-3275869146
That works for me thanks @zeroshade
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(arrow/array): update timestamp json format [arrow-go]
Posted by "efd6 (via GitHub)" <gi...@apache.org>.
efd6 commented on PR #450:
URL: https://github.com/apache/arrow-go/pull/450#issuecomment-3272345874
@erezrokah What would be the default? At this stage any choice would be a breaking change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org