Re: [PR] GH-41594: [Go] Support reading `date64` type & properly validate list-like types [arrow]
zeroshade commented on PR #41595: URL: https://github.com/apache/arrow/pull/41595#issuecomment-2113047575 Just confirming for you @candiduslynx, this did get included in the v16.1.0 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-unsubscr...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] GH-41594: [Go] Support reading `date64` type & properly validate list-like types [arrow]
conbench-apache-arrow[bot] commented on PR #41595: URL: https://github.com/apache/arrow/pull/41595#issuecomment-2101741063 After merging your PR, Conbench analyzed the 5 benchmarking runs that have been run so far on merge-commit 5252c6ce13694fa31dbcb2623d1629cd8fe53a47. There were no benchmark performance regressions. The [full Conbench report](https://github.com/apache/arrow/runs/24757241422) has more details. -- 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-unsubscr...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] GH-41594: [Go] Support reading `date64` type & properly validate list-like types [arrow]
raulcd commented on PR #41595: URL: https://github.com/apache/arrow/pull/41595#issuecomment-2101493858 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 -- 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-unsubscr...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] GH-41594: [Go] Support reading `date64` type & properly validate list-like types [arrow]
candiduslynx commented on PR #41595: URL: https://github.com/apache/arrow/pull/41595#issuecomment-2101368289 > Revision: [b70d470](https://github.com/apache/arrow/commit/b70d4706c844beae2e2bf2489627947baa1cd33c) > > Submitted crossbow builds: [ursacomputing/crossbow @ actions-ef3aae34bf](https://github.com/ursacomputing/crossbow/branches/all?query=actions-ef3aae34bf) > > Task Status > test-debian-12-go-1.21 [](https://github.com/ursacomputing/crossbow/actions/runs/9007660779/job/24747948088) > test-debian-12-go-1.22 [](https://github.com/ursacomputing/crossbow/actions/runs/9007660748/job/24747948109) > verify-rc-source-go-linux-almalinux-8-amd64[](https://github.com/ursacomputing/crossbow/actions/runs/9007660883/job/24747948494) > verify-rc-source-go-linux-conda-latest-amd64 [](https://github.com/ursacomputing/crossbow/actions/runs/9007660715/job/24747948008) > verify-rc-source-go-linux-ubuntu-20.04-amd64 [](https://github.com/ursacomputing/crossbow/actions/runs/9007660604/job/24747947395) > verify-rc-source-go-linux-ubuntu-22.04-amd64 [](https://github.com/ursacomputing/crossbow/actions/runs/9007660557/job/24747947075) > verify-rc-source-go-macos-amd64[](https://github.com/ursacomputing/crossbow/actions/runs/9007660652/job/24747947588) > verify-rc-source-go-macos-arm64[](https://github.com/ursacomputing/crossbow/actions/runs/9007660728/job/24747948012) I think I broke that by removing my fork cc @zeroshade @raulcd -- 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-unsubscr...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] GH-41594: [Go] Support reading `date64` type & properly validate list-like types [arrow]
github-actions[bot] commented on PR #41595: URL: https://github.com/apache/arrow/pull/41595#issuecomment-2101312978 Revision: b70d4706c844beae2e2bf2489627947baa1cd33c Submitted crossbow builds: [ursacomputing/crossbow @ actions-ef3aae34bf](https://github.com/ursacomputing/crossbow/branches/all?query=actions-ef3aae34bf) |Task|Status| ||--| |test-debian-12-go-1.21|[](https://github.com/ursacomputing/crossbow/actions/runs/9007660779/job/24747948088)| |test-debian-12-go-1.22|[](https://github.com/ursacomputing/crossbow/actions/runs/9007660748/job/24747948109)| |verify-rc-source-go-linux-almalinux-8-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/9007660883/job/24747948494)| |verify-rc-source-go-linux-conda-latest-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/9007660715/job/24747948008)| |verify-rc-source-go-linux-ubuntu-20.04-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/9007660604/job/24747947395)| |verify-rc-source-go-linux-ubuntu-22.04-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/9007660557/job/24747947075)| |verify-rc-source-go-macos-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/9007660652/job/24747947588)| |verify-rc-source-go-macos-arm64|[](https://github.com/ursacomputing/crossbow/actions/runs/9007660728/job/24747948012)| -- 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-unsubscr...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] GH-41594: [Go] Support reading `date64` type & properly validate list-like types [arrow]
zeroshade merged PR #41595: URL: https://github.com/apache/arrow/pull/41595 -- 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-unsubscr...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] GH-41594: [Go] Support reading `date64` type & properly validate list-like types [arrow]
raulcd commented on PR #41595: URL: https://github.com/apache/arrow/pull/41595#issuecomment-2101309279 @github-actions crossbow submit *go* -- 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-unsubscr...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] GH-41594: [Go] Support reading `date64` type & properly validate list-like types [arrow]
candiduslynx commented on code in PR #41595:
URL: https://github.com/apache/arrow/pull/41595#discussion_r1594532154
##
go/arrow/csv/reader.go:
##
@@ -982,7 +966,7 @@ func (c conversionColumn) inferType(v string)
arrow.DataType {
c.typ = arrow.FixedWidthTypes.Boolean
case *arrow.BooleanType:
c.typ = arrow.FixedWidthTypes.Date32
- case *arrow.Date32Type:
+ case *arrow.Date32Type, *arrow.Date64Type:
Review Comment:
date32 is fine then
b70d4706c844beae2e2bf2489627947baa1cd33c
--
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-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] GH-41594: [Go] Support reading `date64` type & properly validate list-like types [arrow]
candiduslynx commented on PR #41595: URL: https://github.com/apache/arrow/pull/41595#issuecomment-2101277391 > @candiduslynx can you also add a test for this? a55cd5324d2c47932410b0c7a9c46075386645d2 -- 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-unsubscr...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] GH-41594: [Go] Support reading `date64` type & properly validate list-like types [arrow]
zeroshade commented on PR #41595: URL: https://github.com/apache/arrow/pull/41595#issuecomment-2101257872 @candiduslynx can you also add a test for this? -- 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-unsubscr...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] GH-41594: [Go] Support reading `date64` type & properly validate list-like types [arrow]
zeroshade commented on PR #41595: URL: https://github.com/apache/arrow/pull/41595#issuecomment-2101255987 @raulcd since we're already doing a 16.1.0 release, do you think we could get this into it? -- 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-unsubscr...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] GH-41594: [Go] Support reading `date64` type & properly validate list-like types [arrow]
zeroshade commented on code in PR #41595:
URL: https://github.com/apache/arrow/pull/41595#discussion_r1594515317
##
go/arrow/csv/common.go:
##
@@ -239,21 +239,31 @@ func WithStringsReplacer(replacer *strings.Replacer)
Option {
func validate(schema *arrow.Schema) {
for i, f := range schema.Fields() {
- switch ft := f.Type.(type) {
- case *arrow.BooleanType:
- case *arrow.Int8Type, *arrow.Int16Type, *arrow.Int32Type,
*arrow.Int64Type:
- case *arrow.Uint8Type, *arrow.Uint16Type, *arrow.Uint32Type,
*arrow.Uint64Type:
- case *arrow.Float16Type, *arrow.Float32Type, *arrow.Float64Type:
- case *arrow.StringType, *arrow.LargeStringType:
- case *arrow.TimestampType:
- case *arrow.Date32Type, *arrow.Date64Type:
- case *arrow.Decimal128Type, *arrow.Decimal256Type:
- case *arrow.ListType, *arrow.LargeListType,
*arrow.FixedSizeListType:
- case *arrow.BinaryType, *arrow.LargeBinaryType,
*arrow.FixedSizeBinaryType:
- case arrow.ExtensionType:
- case *arrow.NullType:
- default:
- panic(fmt.Errorf("arrow/csv: field %d (%s) has invalid
data type %T", i, f.Name, ft))
+ if !typeSupported(f.Type) {
+ panic(fmt.Errorf("arrow/csv: field %d (%s) has invalid
data type %T", i, f.Name, f.Type))
}
}
}
+
+func typeSupported(dt arrow.DataType) bool {
+ switch dt := dt.(type) {
+ case *arrow.BooleanType:
+ case *arrow.Int8Type, *arrow.Int16Type, *arrow.Int32Type,
*arrow.Int64Type:
+ case *arrow.Uint8Type, *arrow.Uint16Type, *arrow.Uint32Type,
*arrow.Uint64Type:
+ case *arrow.Float16Type, *arrow.Float32Type, *arrow.Float64Type:
+ case *arrow.StringType, *arrow.LargeStringType:
+ case *arrow.TimestampType:
+ case *arrow.Date32Type, *arrow.Date64Type:
+ case *arrow.Decimal128Type, *arrow.Decimal256Type:
Review Comment:
that makes sense, though it does mean we should probably add those to this
and have them return false like we do for map and other types lol. but that's
not necessary to do in this PR
--
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-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] GH-41594: [Go] Support reading `date64` type & properly validate list-like types [arrow]
candiduslynx commented on PR #41595: URL: https://github.com/apache/arrow/pull/41595#issuecomment-2101247291 @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 -- 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-unsubscr...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] GH-41594: [Go] Support reading `date64` type & properly validate list-like types [arrow]
candiduslynx commented on code in PR #41595:
URL: https://github.com/apache/arrow/pull/41595#discussion_r1594509669
##
go/arrow/csv/common.go:
##
@@ -239,21 +239,31 @@ func WithStringsReplacer(replacer *strings.Replacer)
Option {
func validate(schema *arrow.Schema) {
for i, f := range schema.Fields() {
- switch ft := f.Type.(type) {
- case *arrow.BooleanType:
- case *arrow.Int8Type, *arrow.Int16Type, *arrow.Int32Type,
*arrow.Int64Type:
- case *arrow.Uint8Type, *arrow.Uint16Type, *arrow.Uint32Type,
*arrow.Uint64Type:
- case *arrow.Float16Type, *arrow.Float32Type, *arrow.Float64Type:
- case *arrow.StringType, *arrow.LargeStringType:
- case *arrow.TimestampType:
- case *arrow.Date32Type, *arrow.Date64Type:
- case *arrow.Decimal128Type, *arrow.Decimal256Type:
- case *arrow.ListType, *arrow.LargeListType,
*arrow.FixedSizeListType:
- case *arrow.BinaryType, *arrow.LargeBinaryType,
*arrow.FixedSizeBinaryType:
- case arrow.ExtensionType:
- case *arrow.NullType:
- default:
- panic(fmt.Errorf("arrow/csv: field %d (%s) has invalid
data type %T", i, f.Name, ft))
+ if !typeSupported(f.Type) {
+ panic(fmt.Errorf("arrow/csv: field %d (%s) has invalid
data type %T", i, f.Name, f.Type))
}
}
}
+
+func typeSupported(dt arrow.DataType) bool {
+ switch dt := dt.(type) {
+ case *arrow.BooleanType:
+ case *arrow.Int8Type, *arrow.Int16Type, *arrow.Int32Type,
*arrow.Int64Type:
+ case *arrow.Uint8Type, *arrow.Uint16Type, *arrow.Uint32Type,
*arrow.Uint64Type:
+ case *arrow.Float16Type, *arrow.Float32Type, *arrow.Float64Type:
+ case *arrow.StringType, *arrow.LargeStringType:
+ case *arrow.TimestampType:
+ case *arrow.Date32Type, *arrow.Date64Type:
+ case *arrow.Decimal128Type, *arrow.Decimal256Type:
Review Comment:
I'd say `no`, as this allows explicitly listing what's supported, & won't
fail on the newly added types (string view, list view?)
--
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-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] GH-41594: [Go] Support reading `date64` type & properly validate list-like types [arrow]
candiduslynx commented on code in PR #41595:
URL: https://github.com/apache/arrow/pull/41595#discussion_r1594507030
##
go/arrow/csv/common.go:
##
@@ -239,21 +239,31 @@ func WithStringsReplacer(replacer *strings.Replacer)
Option {
func validate(schema *arrow.Schema) {
for i, f := range schema.Fields() {
- switch ft := f.Type.(type) {
- case *arrow.BooleanType:
- case *arrow.Int8Type, *arrow.Int16Type, *arrow.Int32Type,
*arrow.Int64Type:
- case *arrow.Uint8Type, *arrow.Uint16Type, *arrow.Uint32Type,
*arrow.Uint64Type:
- case *arrow.Float16Type, *arrow.Float32Type, *arrow.Float64Type:
- case *arrow.StringType, *arrow.LargeStringType:
- case *arrow.TimestampType:
- case *arrow.Date32Type, *arrow.Date64Type:
- case *arrow.Decimal128Type, *arrow.Decimal256Type:
- case *arrow.ListType, *arrow.LargeListType,
*arrow.FixedSizeListType:
- case *arrow.BinaryType, *arrow.LargeBinaryType,
*arrow.FixedSizeBinaryType:
- case arrow.ExtensionType:
- case *arrow.NullType:
- default:
- panic(fmt.Errorf("arrow/csv: field %d (%s) has invalid
data type %T", i, f.Name, ft))
+ if !typeSupported(f.Type) {
+ panic(fmt.Errorf("arrow/csv: field %d (%s) has invalid
data type %T", i, f.Name, f.Type))
}
}
}
+
+func typeSupported(dt arrow.DataType) bool {
+ switch dt := dt.(type) {
+ case *arrow.BooleanType:
+ case *arrow.Int8Type, *arrow.Int16Type, *arrow.Int32Type,
*arrow.Int64Type:
+ case *arrow.Uint8Type, *arrow.Uint16Type, *arrow.Uint32Type,
*arrow.Uint64Type:
+ case *arrow.Float16Type, *arrow.Float32Type, *arrow.Float64Type:
+ case *arrow.StringType, *arrow.LargeStringType:
+ case *arrow.TimestampType:
+ case *arrow.Date32Type, *arrow.Date64Type:
+ case *arrow.Decimal128Type, *arrow.Decimal256Type:
Review Comment:
But making an inclusive list might be a good idea, 100%
##
go/arrow/csv/common.go:
##
@@ -239,21 +239,31 @@ func WithStringsReplacer(replacer *strings.Replacer)
Option {
func validate(schema *arrow.Schema) {
for i, f := range schema.Fields() {
- switch ft := f.Type.(type) {
- case *arrow.BooleanType:
- case *arrow.Int8Type, *arrow.Int16Type, *arrow.Int32Type,
*arrow.Int64Type:
- case *arrow.Uint8Type, *arrow.Uint16Type, *arrow.Uint32Type,
*arrow.Uint64Type:
- case *arrow.Float16Type, *arrow.Float32Type, *arrow.Float64Type:
- case *arrow.StringType, *arrow.LargeStringType:
- case *arrow.TimestampType:
- case *arrow.Date32Type, *arrow.Date64Type:
- case *arrow.Decimal128Type, *arrow.Decimal256Type:
- case *arrow.ListType, *arrow.LargeListType,
*arrow.FixedSizeListType:
- case *arrow.BinaryType, *arrow.LargeBinaryType,
*arrow.FixedSizeBinaryType:
- case arrow.ExtensionType:
- case *arrow.NullType:
- default:
- panic(fmt.Errorf("arrow/csv: field %d (%s) has invalid
data type %T", i, f.Name, ft))
+ if !typeSupported(f.Type) {
+ panic(fmt.Errorf("arrow/csv: field %d (%s) has invalid
data type %T", i, f.Name, f.Type))
}
}
}
+
+func typeSupported(dt arrow.DataType) bool {
+ switch dt := dt.(type) {
+ case *arrow.BooleanType:
+ case *arrow.Int8Type, *arrow.Int16Type, *arrow.Int32Type,
*arrow.Int64Type:
+ case *arrow.Uint8Type, *arrow.Uint16Type, *arrow.Uint32Type,
*arrow.Uint64Type:
+ case *arrow.Float16Type, *arrow.Float32Type, *arrow.Float64Type:
+ case *arrow.StringType, *arrow.LargeStringType:
+ case *arrow.TimestampType:
+ case *arrow.Date32Type, *arrow.Date64Type:
+ case *arrow.Decimal128Type, *arrow.Decimal256Type:
Review Comment:
I'd leave this out of scope for now
--
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-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] GH-41594: [Go] Support reading `date64` type & properly validate list-like types [arrow]
zeroshade commented on code in PR #41595:
URL: https://github.com/apache/arrow/pull/41595#discussion_r1594509034
##
go/arrow/csv/common.go:
##
@@ -239,21 +239,31 @@ func WithStringsReplacer(replacer *strings.Replacer)
Option {
func validate(schema *arrow.Schema) {
for i, f := range schema.Fields() {
- switch ft := f.Type.(type) {
- case *arrow.BooleanType:
- case *arrow.Int8Type, *arrow.Int16Type, *arrow.Int32Type,
*arrow.Int64Type:
- case *arrow.Uint8Type, *arrow.Uint16Type, *arrow.Uint32Type,
*arrow.Uint64Type:
- case *arrow.Float16Type, *arrow.Float32Type, *arrow.Float64Type:
- case *arrow.StringType, *arrow.LargeStringType:
- case *arrow.TimestampType:
- case *arrow.Date32Type, *arrow.Date64Type:
- case *arrow.Decimal128Type, *arrow.Decimal256Type:
- case *arrow.ListType, *arrow.LargeListType,
*arrow.FixedSizeListType:
- case *arrow.BinaryType, *arrow.LargeBinaryType,
*arrow.FixedSizeBinaryType:
- case arrow.ExtensionType:
- case *arrow.NullType:
- default:
- panic(fmt.Errorf("arrow/csv: field %d (%s) has invalid
data type %T", i, f.Name, ft))
+ if !typeSupported(f.Type) {
+ panic(fmt.Errorf("arrow/csv: field %d (%s) has invalid
data type %T", i, f.Name, f.Type))
}
}
}
+
+func typeSupported(dt arrow.DataType) bool {
+ switch dt := dt.(type) {
+ case *arrow.BooleanType:
+ case *arrow.Int8Type, *arrow.Int16Type, *arrow.Int32Type,
*arrow.Int64Type:
+ case *arrow.Uint8Type, *arrow.Uint16Type, *arrow.Uint32Type,
*arrow.Uint64Type:
+ case *arrow.Float16Type, *arrow.Float32Type, *arrow.Float64Type:
+ case *arrow.StringType, *arrow.LargeStringType:
+ case *arrow.TimestampType:
+ case *arrow.Date32Type, *arrow.Date64Type:
+ case *arrow.Decimal128Type, *arrow.Decimal256Type:
Review Comment:
that's fine. as long as the tests still pass and all is good, I'm fine with
cleaning this up as a follow-up
--
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-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] GH-41594: [Go] Support reading `date64` type & properly validate list-like types [arrow]
zeroshade commented on code in PR #41595:
URL: https://github.com/apache/arrow/pull/41595#discussion_r1594508222
##
go/arrow/csv/reader.go:
##
@@ -982,7 +966,7 @@ func (c conversionColumn) inferType(v string)
arrow.DataType {
c.typ = arrow.FixedWidthTypes.Boolean
case *arrow.BooleanType:
c.typ = arrow.FixedWidthTypes.Date32
- case *arrow.Date32Type:
+ case *arrow.Date32Type, *arrow.Date64Type:
Review Comment:
The way the inferring works is essentially a loop that progressively tries
the next type in that switch case. since both Date32 and Date64 expect the data
to be in the same format it makes sense to only pick one for our inferring.
So the question is which one do we want the default inferred type to be when
it sees "-MM-DD" data? Date32 or Date64? whichever one we want the inferred
default to be, that's the one we leave in this switch (and should be what
`c.typ` is set to at line 968 above it
--
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-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] GH-41594: [Go] Support reading `date64` type & properly validate list-like types [arrow]
candiduslynx commented on code in PR #41595:
URL: https://github.com/apache/arrow/pull/41595#discussion_r1594507030
##
go/arrow/csv/common.go:
##
@@ -239,21 +239,31 @@ func WithStringsReplacer(replacer *strings.Replacer)
Option {
func validate(schema *arrow.Schema) {
for i, f := range schema.Fields() {
- switch ft := f.Type.(type) {
- case *arrow.BooleanType:
- case *arrow.Int8Type, *arrow.Int16Type, *arrow.Int32Type,
*arrow.Int64Type:
- case *arrow.Uint8Type, *arrow.Uint16Type, *arrow.Uint32Type,
*arrow.Uint64Type:
- case *arrow.Float16Type, *arrow.Float32Type, *arrow.Float64Type:
- case *arrow.StringType, *arrow.LargeStringType:
- case *arrow.TimestampType:
- case *arrow.Date32Type, *arrow.Date64Type:
- case *arrow.Decimal128Type, *arrow.Decimal256Type:
- case *arrow.ListType, *arrow.LargeListType,
*arrow.FixedSizeListType:
- case *arrow.BinaryType, *arrow.LargeBinaryType,
*arrow.FixedSizeBinaryType:
- case arrow.ExtensionType:
- case *arrow.NullType:
- default:
- panic(fmt.Errorf("arrow/csv: field %d (%s) has invalid
data type %T", i, f.Name, ft))
+ if !typeSupported(f.Type) {
+ panic(fmt.Errorf("arrow/csv: field %d (%s) has invalid
data type %T", i, f.Name, f.Type))
}
}
}
+
+func typeSupported(dt arrow.DataType) bool {
+ switch dt := dt.(type) {
+ case *arrow.BooleanType:
+ case *arrow.Int8Type, *arrow.Int16Type, *arrow.Int32Type,
*arrow.Int64Type:
+ case *arrow.Uint8Type, *arrow.Uint16Type, *arrow.Uint32Type,
*arrow.Uint64Type:
+ case *arrow.Float16Type, *arrow.Float32Type, *arrow.Float64Type:
+ case *arrow.StringType, *arrow.LargeStringType:
+ case *arrow.TimestampType:
+ case *arrow.Date32Type, *arrow.Date64Type:
+ case *arrow.Decimal128Type, *arrow.Decimal256Type:
Review Comment:
But making an inclusive list might be a good idea, 100%
--
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-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] GH-41594: [Go] Support reading `date64` type & properly validate list-like types [arrow]
zeroshade commented on code in PR #41595:
URL: https://github.com/apache/arrow/pull/41595#discussion_r1594506272
##
go/arrow/csv/reader.go:
##
@@ -740,81 +740,67 @@ func (r *Reader) parseDate32(field array.Builder, str
string) {
field.(*array.Date32Builder).Append(arrow.Date32FromTime(tm))
}
-func (r *Reader) parseTime32(field array.Builder, str string, unit
arrow.TimeUnit) {
+func (r *Reader) parseDate64(field array.Builder, str string) {
if r.isNull(str) {
field.AppendNull()
return
}
- val, err := arrow.Time32FromString(str, unit)
+ tm, err := time.Parse("2006-01-02", str)
Review Comment:
sure. if anyone ends up wanting that it can just be a follow-up enhancement
--
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-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] GH-41594: [Go] Support reading `date64` type & properly validate list-like types [arrow]
candiduslynx commented on code in PR #41595:
URL: https://github.com/apache/arrow/pull/41595#discussion_r1594506201
##
go/arrow/csv/common.go:
##
@@ -239,21 +239,31 @@ func WithStringsReplacer(replacer *strings.Replacer)
Option {
func validate(schema *arrow.Schema) {
for i, f := range schema.Fields() {
- switch ft := f.Type.(type) {
- case *arrow.BooleanType:
- case *arrow.Int8Type, *arrow.Int16Type, *arrow.Int32Type,
*arrow.Int64Type:
- case *arrow.Uint8Type, *arrow.Uint16Type, *arrow.Uint32Type,
*arrow.Uint64Type:
- case *arrow.Float16Type, *arrow.Float32Type, *arrow.Float64Type:
- case *arrow.StringType, *arrow.LargeStringType:
- case *arrow.TimestampType:
- case *arrow.Date32Type, *arrow.Date64Type:
- case *arrow.Decimal128Type, *arrow.Decimal256Type:
- case *arrow.ListType, *arrow.LargeListType,
*arrow.FixedSizeListType:
- case *arrow.BinaryType, *arrow.LargeBinaryType,
*arrow.FixedSizeBinaryType:
- case arrow.ExtensionType:
- case *arrow.NullType:
- default:
- panic(fmt.Errorf("arrow/csv: field %d (%s) has invalid
data type %T", i, f.Name, ft))
+ if !typeSupported(f.Type) {
+ panic(fmt.Errorf("arrow/csv: field %d (%s) has invalid
data type %T", i, f.Name, f.Type))
}
}
}
+
+func typeSupported(dt arrow.DataType) bool {
+ switch dt := dt.(type) {
+ case *arrow.BooleanType:
+ case *arrow.Int8Type, *arrow.Int16Type, *arrow.Int32Type,
*arrow.Int64Type:
+ case *arrow.Uint8Type, *arrow.Uint16Type, *arrow.Uint32Type,
*arrow.Uint64Type:
+ case *arrow.Float16Type, *arrow.Float32Type, *arrow.Float64Type:
+ case *arrow.StringType, *arrow.LargeStringType:
+ case *arrow.TimestampType:
+ case *arrow.Date32Type, *arrow.Date64Type:
+ case *arrow.Decimal128Type, *arrow.Decimal256Type:
Review Comment:
I'd leave this out of scope for now
--
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-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] GH-41594: [Go] Support reading `date64` type & properly validate list-like types [arrow]
candiduslynx commented on code in PR #41595:
URL: https://github.com/apache/arrow/pull/41595#discussion_r1594505828
##
go/arrow/csv/reader.go:
##
@@ -982,7 +966,7 @@ func (c conversionColumn) inferType(v string)
arrow.DataType {
c.typ = arrow.FixedWidthTypes.Boolean
case *arrow.BooleanType:
c.typ = arrow.FixedWidthTypes.Date32
- case *arrow.Date32Type:
+ case *arrow.Date32Type, *arrow.Date64Type:
Review Comment:
should I add it to the inferring somewhere? (maybe I missed something)
--
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-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] GH-41594: [Go] Support reading `date64` type & properly validate list-like types [arrow]
candiduslynx commented on code in PR #41595:
URL: https://github.com/apache/arrow/pull/41595#discussion_r1594505197
##
go/arrow/csv/reader.go:
##
@@ -740,81 +740,67 @@ func (r *Reader) parseDate32(field array.Builder, str
string) {
field.(*array.Date32Builder).Append(arrow.Date32FromTime(tm))
}
-func (r *Reader) parseTime32(field array.Builder, str string, unit
arrow.TimeUnit) {
+func (r *Reader) parseDate64(field array.Builder, str string) {
if r.isNull(str) {
field.AppendNull()
return
}
- val, err := arrow.Time32FromString(str, unit)
+ tm, err := time.Parse("2006-01-02", str)
Review Comment:
I don't think so, as the marshalling to csv uses this format (same as
date32, so out of scope?)
--
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-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] GH-41594: [Go] Support reading `date64` type & properly validate list-like types [arrow]
zeroshade commented on code in PR #41595:
URL: https://github.com/apache/arrow/pull/41595#discussion_r1594503371
##
go/arrow/csv/reader.go:
##
@@ -982,7 +966,7 @@ func (c conversionColumn) inferType(v string)
arrow.DataType {
c.typ = arrow.FixedWidthTypes.Boolean
case *arrow.BooleanType:
c.typ = arrow.FixedWidthTypes.Date32
- case *arrow.Date32Type:
+ case *arrow.Date32Type, *arrow.Date64Type:
Review Comment:
since we never try to use `Date64Type` in this list of types for inferring,
i don't think this gains us anything
--
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-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] GH-41594: [Go] Support reading `date64` type & properly validate list-like types [arrow]
zeroshade commented on code in PR #41595:
URL: https://github.com/apache/arrow/pull/41595#discussion_r1594501276
##
go/arrow/csv/reader.go:
##
@@ -740,81 +740,67 @@ func (r *Reader) parseDate32(field array.Builder, str
string) {
field.(*array.Date32Builder).Append(arrow.Date32FromTime(tm))
}
-func (r *Reader) parseTime32(field array.Builder, str string, unit
arrow.TimeUnit) {
+func (r *Reader) parseDate64(field array.Builder, str string) {
if r.isNull(str) {
field.AppendNull()
return
}
- val, err := arrow.Time32FromString(str, unit)
+ tm, err := time.Parse("2006-01-02", str)
Review Comment:
do we want to allow specifying the date format?
--
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-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] GH-41594: [Go] Support reading `date64` type & properly validate list-like types [arrow]
zeroshade commented on code in PR #41595:
URL: https://github.com/apache/arrow/pull/41595#discussion_r1594499804
##
go/arrow/csv/common.go:
##
@@ -239,21 +239,31 @@ func WithStringsReplacer(replacer *strings.Replacer)
Option {
func validate(schema *arrow.Schema) {
for i, f := range schema.Fields() {
- switch ft := f.Type.(type) {
- case *arrow.BooleanType:
- case *arrow.Int8Type, *arrow.Int16Type, *arrow.Int32Type,
*arrow.Int64Type:
- case *arrow.Uint8Type, *arrow.Uint16Type, *arrow.Uint32Type,
*arrow.Uint64Type:
- case *arrow.Float16Type, *arrow.Float32Type, *arrow.Float64Type:
- case *arrow.StringType, *arrow.LargeStringType:
- case *arrow.TimestampType:
- case *arrow.Date32Type, *arrow.Date64Type:
- case *arrow.Decimal128Type, *arrow.Decimal256Type:
- case *arrow.ListType, *arrow.LargeListType,
*arrow.FixedSizeListType:
- case *arrow.BinaryType, *arrow.LargeBinaryType,
*arrow.FixedSizeBinaryType:
- case arrow.ExtensionType:
- case *arrow.NullType:
- default:
- panic(fmt.Errorf("arrow/csv: field %d (%s) has invalid
data type %T", i, f.Name, ft))
+ if !typeSupported(f.Type) {
+ panic(fmt.Errorf("arrow/csv: field %d (%s) has invalid
data type %T", i, f.Name, f.Type))
}
}
}
+
+func typeSupported(dt arrow.DataType) bool {
+ switch dt := dt.(type) {
+ case *arrow.BooleanType:
+ case *arrow.Int8Type, *arrow.Int16Type, *arrow.Int32Type,
*arrow.Int64Type:
+ case *arrow.Uint8Type, *arrow.Uint16Type, *arrow.Uint32Type,
*arrow.Uint64Type:
+ case *arrow.Float16Type, *arrow.Float32Type, *arrow.Float64Type:
+ case *arrow.StringType, *arrow.LargeStringType:
+ case *arrow.TimestampType:
+ case *arrow.Date32Type, *arrow.Date64Type:
+ case *arrow.Decimal128Type, *arrow.Decimal256Type:
Review Comment:
should we bother actively listing these out? it might make more sense to
just explicitly have cases for the types we don't support to return false and
then just default return true?
just to shrink this
--
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-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] GH-41594: [Go] Support reading `date64` type & properly validate list-like types [arrow]
candiduslynx commented on PR #41595: URL: https://github.com/apache/arrow/pull/41595#issuecomment-2101070846 @zeroshade I've been reviewing one of our libraries & found this. -- 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-unsubscr...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
