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.

Sign up for GitHub

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

Jump to bottom

GH-41594: [Go] Support reading date64 type & properly validate list-like types #41595

Merged
merged 4 commits into from
May 8, 2024
Merged

GH-41594: [Go] Support reading date64 type & properly validate list-like types #41595

merged 4 commits into from
May 8, 2024

Conversation

candiduslynx
Copy link
Contributor

@candiduslynx candiduslynx commented May 8, 2024 •
edited
Loading

This PR includes 2 fixes:

  1. support reading date64 columns (as write is supported)
  2. properly validate list-like data types (list of unsupported is unsupported)

Rationale for this change

See #41594

What changes are included in this PR?

  1. Added date64 reading & conversion funcs similar to date32
  2. Refactored date type validation

Are these changes tested?

a55cd53

Are there any user-facing changes?

No.

@candiduslynx
Copy link
Contributor Author

@zeroshade I've been reviewing one of our libraries & found this.

@github-actions github-actions bot added the awaiting review Awaiting review label May 8, 2024
@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels May 8, 2024
zeroshade
zeroshade reviewed May 8, 2024
View reviewed changes
go/arrow/csv/reader.go Outdated Show resolved Hide resolved
zeroshade
zeroshade reviewed May 8, 2024
View reviewed changes
Copy link
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.

overall this looks great, just a few nitpicks

@candiduslynx
Copy link
Contributor Author

@zeroshade I'd like to also ask to port it to v16, as we're using it & it's a blocker to wait for the v17 release

@zeroshade
Copy link
Member

@raulcd since we're already doing a 16.1.0 release, do you think we could get this into it?

@zeroshade
Copy link
Member

@candiduslynx can you also add a test for this?

@candiduslynx
Copy link
Contributor Author

@candiduslynx can you also add a test for this?

a55cd53

@github-actions github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels May 8, 2024
zeroshade
zeroshade approved these changes May 8, 2024
View reviewed changes
Copy link
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.

LGTM gonna wait to merge until I hear back from @raulcd about potentially adding this to the 16.1.0 release

@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels May 8, 2024
@raulcd
Copy link
Member

raulcd commented May 8, 2024

@github-actions crossbow submit go

@zeroshade zeroshade merged commit 5252c6c into apache:main May 8, 2024
25 of 26 checks passed
@zeroshade zeroshade removed the awaiting merge Awaiting merge label May 8, 2024
@zeroshade zeroshade mentioned this pull request May 8, 2024
Closed
@candiduslynx candiduslynx deleted the fix/go/csv/types branch May 8, 2024 19:47
@github-actions GitHub Actions
Copy link

github-actions bot commented May 8, 2024

Revision: b70d470

Submitted crossbow builds: ursacomputing/crossbow @ actions-ef3aae34bf

Task Status
test-debian-12-go-1.21 GitHub Actions
test-debian-12-go-1.22 GitHub Actions
verify-rc-source-go-linux-almalinux-8-amd64 GitHub Actions
verify-rc-source-go-linux-conda-latest-amd64 GitHub Actions
verify-rc-source-go-linux-ubuntu-20.04-amd64 GitHub Actions
verify-rc-source-go-linux-ubuntu-22.04-amd64 GitHub Actions
verify-rc-source-go-macos-amd64 GitHub Actions
verify-rc-source-go-macos-arm64 GitHub Actions

@candiduslynx
Copy link
Contributor Author

Revision: b70d470

Submitted crossbow builds: ursacomputing/crossbow @ actions-ef3aae34bf

Task Status
test-debian-12-go-1.21 GitHub Actions
test-debian-12-go-1.22 GitHub Actions
verify-rc-source-go-linux-almalinux-8-amd64 GitHub Actions
verify-rc-source-go-linux-conda-latest-amd64 GitHub Actions
verify-rc-source-go-linux-ubuntu-20.04-amd64 GitHub Actions
verify-rc-source-go-linux-ubuntu-22.04-amd64 GitHub Actions
verify-rc-source-go-macos-amd64 GitHub Actions
verify-rc-source-go-macos-arm64 GitHub Actions

I think I broke that by removing my fork 😄
cc @zeroshade @raulcd

@raulcd
Copy link
Member

raulcd commented May 8, 2024

no worries, I'll test this on the maintenance branch release before creating a release candidate just to validate none of the release jobs break

@conbench-apache-arrow conbench-apache-arrow
Copy link

After merging your PR, Conbench analyzed the 5 benchmarking runs that have been run so far on merge-commit 5252c6c.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

@candiduslynx candiduslynx mentioned this pull request May 9, 2024
Closed
raulcd pushed a commit that referenced this pull request May 9, 2024
@candiduslynx @raulcd
GH-41594: [Go] Support reading date64 type & properly validate list…
5874287 
…-like types (#41595)

This PR includes 2 fixes:
1. support reading `date64` columns (as write is supported)
2. properly validate list-like data types (list of unsupported is unsupported)

### Rationale for this change

See #41594

### What changes are included in this PR?

1. Added `date64` reading & conversion funcs similar to `date32`
2. Refactored date type validation

### Are these changes tested?

a55cd53

### Are there any user-facing changes?

No.

* GitHub Issue: #41594

Authored-by: candiduslynx <candiduslynx@gmail.com>
Signed-off-by: Matt Topol <zotthewizard@gmail.com>
@zeroshade
Copy link
Member

Just confirming for you @candiduslynx, this did get included in the v16.1.0 release

vibhatha pushed a commit to vibhatha/arrow that referenced this pull request May 25, 2024
@candiduslynx @vibhatha
apacheGH-41594: [Go] Support reading date64 type & properly validat…
ee4fb91 
…e list-like types (apache#41595)

This PR includes 2 fixes:
1. support reading `date64` columns (as write is supported)
2. properly validate list-like data types (list of unsupported is unsupported)

### Rationale for this change

See apache#41594

### What changes are included in this PR?

1. Added `date64` reading & conversion funcs similar to `date32`
2. Refactored date type validation

### Are these changes tested?

a55cd53

### Are there any user-facing changes?

No.

* GitHub Issue: apache#41594

Authored-by: candiduslynx <candiduslynx@gmail.com>
Signed-off-by: Matt Topol <zotthewizard@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zeroshade zeroshade zeroshade approved these changes

Assignees
No one assigned
Labels
Component: Go
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@candiduslynx @zeroshade @raulcd