-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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.
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
[Format][FlightSQL] Spec refers to "int" type (which seems Java specific) rather than "int32" #35118
Comments
…rather than `int`
Also related to #35107 |
…than `int` (#35120) ### Rationale for this change The spec is inconsistent -- see details on #35118 ### What changes are included in this PR? Use `int32` to refer to 32-bit integers rather than `int` ### Are these changes tested? No, only comments are changed ### Are there any user-facing changes? This clarifies a small corner case in the document * Closes: #35118 * Closes: #35118 Authored-by: Chunchun <[email protected]> Signed-off-by: Andrew Lamb <[email protected]>
There are more to change from Lines 1067 to 1112 in 00072f9
Go uses arrow/go/arrow/flight/flightsql/schema_ref/reference_schemas.go Lines 87 to 104 in 00072f9
C++ uses arrow/cpp/src/arrow/flight/sql/server.cc Lines 1186 to 1203 in 00072f9
Java uses arrow/java/flight/flight-sql/src/main/java/org/apache/arrow/flight/sql/FlightSqlProducer.java Lines 882 to 901 in 00072f9
As mentioned in the above, |
…egers rather than `int` (#35213) ### Rationale for this change There are more inconsistency of the spec format found -- see details on the original issue #35118. #35120 is the first PR with the same fix. ### What changes are included in this PR? Use `int32` to refer to 32-bit integers rather than `int` ### Are these changes tested? No, only comments are changed ### Are there any user-facing changes? This clarifies a small corner case in the document * Closes: #35118 Authored-by: Chunchun <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>
Note this was fixed in #35213 -- thanks @appletreeisyellow and @kou |
…ather than `int` (apache#35120) ### Rationale for this change The spec is inconsistent -- see details on apache#35118 ### What changes are included in this PR? Use `int32` to refer to 32-bit integers rather than `int` ### Are these changes tested? No, only comments are changed ### Are there any user-facing changes? This clarifies a small corner case in the document * Closes: apache#35118 * Closes: apache#35118 Authored-by: Chunchun <[email protected]> Signed-off-by: Andrew Lamb <[email protected]>
…it integers rather than `int` (apache#35213) ### Rationale for this change There are more inconsistency of the spec format found -- see details on the original issue apache#35118. apache#35120 is the first PR with the same fix. ### What changes are included in this PR? Use `int32` to refer to 32-bit integers rather than `int` ### Are these changes tested? No, only comments are changed ### Are there any user-facing changes? This clarifies a small corner case in the document * Closes: apache#35118 Authored-by: Chunchun <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>
…ather than `int` (apache#35120) ### Rationale for this change The spec is inconsistent -- see details on apache#35118 ### What changes are included in this PR? Use `int32` to refer to 32-bit integers rather than `int` ### Are these changes tested? No, only comments are changed ### Are there any user-facing changes? This clarifies a small corner case in the document * Closes: apache#35118 * Closes: apache#35118 Authored-by: Chunchun <[email protected]> Signed-off-by: Andrew Lamb <[email protected]>
…it integers rather than `int` (apache#35213) ### Rationale for this change There are more inconsistency of the spec format found -- see details on the original issue apache#35118. apache#35120 is the first PR with the same fix. ### What changes are included in this PR? Use `int32` to refer to 32-bit integers rather than `int` ### Are these changes tested? No, only comments are changed ### Are there any user-facing changes? This clarifies a small corner case in the document * Closes: apache#35118 Authored-by: Chunchun <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>
…ather than `int` (apache#35120) ### Rationale for this change The spec is inconsistent -- see details on apache#35118 ### What changes are included in this PR? Use `int32` to refer to 32-bit integers rather than `int` ### Are these changes tested? No, only comments are changed ### Are there any user-facing changes? This clarifies a small corner case in the document * Closes: apache#35118 * Closes: apache#35118 Authored-by: Chunchun <[email protected]> Signed-off-by: Andrew Lamb <[email protected]>
…it integers rather than `int` (apache#35213) ### Rationale for this change There are more inconsistency of the spec format found -- see details on the original issue apache#35118. apache#35120 is the first PR with the same fix. ### What changes are included in this PR? Use `int32` to refer to 32-bit integers rather than `int` ### Are these changes tested? No, only comments are changed ### Are there any user-facing changes? This clarifies a small corner case in the document * Closes: apache#35118 Authored-by: Chunchun <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>
Describe the bug, including details regarding any error messages, version, and platform.
While implement FlightSQL in our project, we found that the FlightSql.proto format reference refers to a type
int
arrow/format/FlightSql.proto
Line 1370 in c03ca8f
What
int
means was slightly unclear as the Arrow spec refers to "Int" types withbitWidth
of 8, 16, 32 or 64 but the rust implementation has Int8, Int16, Int32 and Int64.arrow/format/Schema.fbs
Lines 145 to 148 in c03ca8f
Go uses
Int32
:arrow/go/arrow/flight/flightsql/schema_ref/reference_schemas.go
Line 63 in c03ca8f
C++ uses
Int32
:arrow/cpp/src/arrow/flight/sql/server.cc
Line 1144 in c03ca8f
Java uses
INT
:arrow/java/flight/flight-sql/src/main/java/org/apache/arrow/flight/sql/FlightSqlProducer.java
Line 848 in c03ca8f
which is defined at:
arrow/java/adapter/jdbc/src/test/java/org/apache/arrow/adapter/jdbc/h2/JdbcToArrowDataTypesTest.java
Line 90 in c03ca8f
According to The Java Language Specification from Section 4.2: Primitive Types and Values:
Therefore, we can conclude that
INT
in arrow Java refers to 32-bit integer.Proposal
I propose that we change the proto spec to be consistent and use
int<bitwidth>
to refer to integer types. So specifically, changeint
toint32
to match C++/Rust/Go as well as the convention in the rest of Flight.protoHere is the reference to details of our discussion https://github.com/influxdata/influxdb_iox/pull/7546#discussion_r1165830088
Component(s)
Format
The text was updated successfully, but these errors were encountered: