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.

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 "uint1" type (which seems Java specific) rather than "uint8" #35107

Closed
alamb opened this issue Apr 13, 2023 · 0 comments · Fixed by #35108
Closed

Comments

@alamb
Copy link
Contributor

alamb commented Apr 13, 2023

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 unit1 (not uint16 or uint8)

arrow/format/FlightSql.proto

Lines 1327 to 1328 in 6432a23

* update_rule: uint1 not null,
* delete_rule: uint1 not null

What uint1 means was slightly unclear as the Arrow spec refers to "Int" types with bitWidth of 8, 16, 32 or 64 but the rust implementation has UInt8, UInt16, UInt32 and UInt64.

arrow/format/Schema.fbs

Lines 145 to 148 in 6432a23

table Int {
bitWidth: int; // restricted to 8, 16, 32, and 64 in v1
is_signed: bool;
}

Go uses uint8:

{Name: "update_rule", Type: arrow.PrimitiveTypes.Uint8, Nullable: false},

C++ uses uint8:

field("pk_key_name", utf8(), true), field("update_rule", uint8(), false),

Java uses "UINT1"

Field.notNullable("update_rule", MinorType.UINT1.getType()),
Field.notNullable("delete_rule", MinorType.UINT1.getType())));

Reading the Java Arrow implementation, it appears to use the term UINT1 for 8 bit integers:

BufferReader UINT1 = new BufferReader() {
@Override
protected ArrowBuf read(BufferAllocator allocator, int count) throws IOException {
final long size = (long) count * TinyIntVector.TYPE_WIDTH;
ArrowBuf buf = allocator.buffer(size);
for (int i = 0; i < count; i++) {
parser.nextToken();
buf.writeByte(parser.getShortValue() & 0xFF);
}
return buf;
}
};

However, inconsistently, while other parts of the flightsql spec refer to uint32

* info_name: uint32 not null,

The Java code uses UINT4 for this

Proposal

I propose that we change the proto spec to be consistent and use uint<bitwidth> to refer to integer types. So specifically, change uint1 to uint8 to match C++/Rust/Go as well as the convention in the rest of Flight.proto

Here is the reference to details of our discussion https://github.com/influxdata/influxdb_iox/pull/7532/files#r1165307397

Component(s)

Format

@alamb alamb self-assigned this Apr 13, 2023
@alamb alamb changed the title FlightSQL.proto document refers to "uint1" type (which seems Java specific) FlightSQL.proto document refers to "uint1" type (which seems Java specific) rather than "uint8" Apr 13, 2023
@alamb alamb changed the title FlightSQL.proto document refers to "uint1" type (which seems Java specific) rather than "uint8" [FLIGHTSQL] Spec refers to "uint1" type (which seems Java specific) rather than "uint8" Apr 13, 2023
@alamb alamb changed the title [FLIGHTSQL] Spec refers to "uint1" type (which seems Java specific) rather than "uint8" [Format][FlightSQL] Spec refers to "uint1" type (which seems Java specific) rather than "uint8" Apr 13, 2023
alamb added a commit that referenced this issue Apr 16, 2023
…s rather than `uint1` (#35108)

### Rationale for this change

The spec is inconsistent -- see details on #35107

### What changes are included in this PR?

Use `uint8` to refer to 8 bit unsigned integers rather than `uint1`

### Are these changes tested?
No, they are comment only changed

### Are there any user-facing changes?

This clarifies a small corner case in the document
* Closes: #35107

Authored-by: Andrew Lamb <andrew@nerdnetworks.org>
Signed-off-by: Andrew Lamb <andrew@nerdnetworks.org>
@alamb alamb added this to the 13.0.0 milestone Apr 16, 2023
liujiacheng777 pushed a commit to LoongArch-Python/arrow that referenced this issue May 11, 2023
…ntegers rather than `uint1` (apache#35108)

### Rationale for this change

The spec is inconsistent -- see details on apache#35107

### What changes are included in this PR?

Use `uint8` to refer to 8 bit unsigned integers rather than `uint1`

### Are these changes tested?
No, they are comment only changed

### Are there any user-facing changes?

This clarifies a small corner case in the document
* Closes: apache#35107

Authored-by: Andrew Lamb <andrew@nerdnetworks.org>
Signed-off-by: Andrew Lamb <andrew@nerdnetworks.org>
ArgusLi pushed a commit to Bit-Quill/arrow that referenced this issue May 15, 2023
…ntegers rather than `uint1` (apache#35108)

### Rationale for this change

The spec is inconsistent -- see details on apache#35107

### What changes are included in this PR?

Use `uint8` to refer to 8 bit unsigned integers rather than `uint1`

### Are these changes tested?
No, they are comment only changed

### Are there any user-facing changes?

This clarifies a small corner case in the document
* Closes: apache#35107

Authored-by: Andrew Lamb <andrew@nerdnetworks.org>
Signed-off-by: Andrew Lamb <andrew@nerdnetworks.org>
rtpsw pushed a commit to rtpsw/arrow that referenced this issue May 16, 2023
…ntegers rather than `uint1` (apache#35108)

### Rationale for this change

The spec is inconsistent -- see details on apache#35107

### What changes are included in this PR?

Use `uint8` to refer to 8 bit unsigned integers rather than `uint1`

### Are these changes tested?
No, they are comment only changed

### Are there any user-facing changes?

This clarifies a small corner case in the document
* Closes: apache#35107

Authored-by: Andrew Lamb <andrew@nerdnetworks.org>
Signed-off-by: Andrew Lamb <andrew@nerdnetworks.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant