charlesdong1991 commented on code in PR #638:
URL: https://github.com/apache/fluss-rust/pull/638#discussion_r3521446058
##########
website/docs/user-guide/cpp/data-types.md:
##########
@@ -85,12 +85,9 @@ auto schema = fluss::Schema::NewBuilder()
.Build();
```
-:::note Arrow escape hatch
-If you already have an `arrow::Schema`, pass it directly with
-`fluss::Schema::FromArrow(arrow_schema, /*primary_keys=*/{"id"})`. It's
-equivalent — the native factories above lower to the same Arrow types
-internally, without pulling Arrow into your code.
-:::
+Column types are sent to the server exactly as declared: precision, scale,
+length, per-level nullability, and `ROW` field names all round-trip losslessly
Review Comment:
does it hold true for Time? i think precision will change still for time?
##########
bindings/cpp/src/types.rs:
##########
@@ -44,229 +47,184 @@ pub const DATA_TYPE_ARRAY: i32 = 17;
pub const DATA_TYPE_MAP: i32 = 18;
pub const DATA_TYPE_ROW: i32 = 19;
-/// Separates scalar and array type specs so each variant only carries
-/// the fields it actually needs — no zeroed-out placeholders.
-enum FfiDataTypeSpec {
- Scalar {
- data_type: i32,
- precision: u32,
- scale: u32,
- nullable: bool,
- },
- Array {
- element_data_type: i32,
- element_precision: u32,
- element_scale: u32,
- array_nesting: u32,
- /// `nesting` entries for each ARRAY wrapper (outermost first) plus
- /// one trailing entry for the leaf scalar. Length = `nesting + 1`.
- array_nullability: Vec<u8>,
- },
+fn ffi_column_to_core_data_type(col: &ffi::FfiColumn) -> Result<DataType> {
+ let mut cursor = 0usize;
+ let dt = nodes_to_data_type(&col.type_nodes, &mut cursor)?;
+ if cursor != col.type_nodes.len() {
+ return Err(anyhow!(
+ "Column '{}': type tree has {} trailing nodes",
+ col.name,
+ col.type_nodes.len() - cursor
+ ));
+ }
+ Ok(dt)
}
-fn ffi_column_to_core_data_type(col: &ffi::FfiColumn) ->
Result<fcore::metadata::DataType> {
- if col.data_type == DATA_TYPE_ARRAY {
- ffi_data_type_to_core(FfiDataTypeSpec::Array {
- element_data_type: col.element_data_type,
- element_precision: col.element_precision as u32,
- element_scale: col.element_scale as u32,
- array_nesting: col.array_nesting.max(0) as u32,
- array_nullability: col.array_nullability.clone(),
- })
- } else {
- ffi_data_type_to_core(FfiDataTypeSpec::Scalar {
- data_type: col.data_type,
- precision: col.precision as u32,
- scale: col.scale as u32,
- nullable: col.nullable,
- })
+/// Reconstruct one type from a preorder node arena, advancing `cursor` past
+/// the consumed nodes. Mirrors the C++ `data_type_to_nodes` encoder.
+fn nodes_to_data_type(nodes: &[ffi::FfiTypeNode], cursor: &mut usize) ->
Result<DataType> {
+ let node = nodes
+ .get(*cursor)
+ .ok_or_else(|| anyhow!("type tree ended before all nodes were read"))?;
+ *cursor += 1;
+
+ if node.precision < 0 || node.scale < 0 {
+ return Err(anyhow!(
+ "type node precision and scale must be non-negative"
+ ));
}
+ let precision = node.precision as u32;
+ let scale = node.scale as u32;
+
+ let dt = match node.type_id {
+ DATA_TYPE_ARRAY => {
+ let element = nodes_to_data_type(nodes, cursor)?;
+ DataType::Array(ArrayType::with_nullable(node.nullable, element))
+ }
+ DATA_TYPE_MAP => {
+ let key = nodes_to_data_type(nodes, cursor)?;
+ let value = nodes_to_data_type(nodes, cursor)?;
+ DataType::Map(MapType::with_nullable(node.nullable, key, value))
+ }
+ DATA_TYPE_ROW => {
+ let mut fields = Vec::with_capacity(node.child_count as usize);
+ for _ in 0..node.child_count {
+ let field_name = nodes
+ .get(*cursor)
+ .ok_or_else(|| anyhow!("ROW field missing from type
tree"))?
+ .field_name
+ .clone();
+ let field_type = nodes_to_data_type(nodes, cursor)?;
+ fields.push(DataField::new(field_name, field_type, None));
Review Comment:
so we will drop description? i guess it is intended?
##########
bindings/cpp/test/test_ffi_converter.cpp:
##########
@@ -18,223 +18,220 @@
*/
#include <gtest/gtest.h>
+
#include <stdexcept>
#include "ffi_converter.hpp"
namespace {
-fluss::ffi::FfiColumn MakeArrayColumn(int32_t nesting, int32_t element_type,
- bool nullable = true, bool leaf_nullable
= true,
- std::vector<uint8_t>
per_level_nullability = {}) {
- fluss::ffi::FfiColumn col;
- col.name = rust::String("bad_array");
- col.data_type = static_cast<int32_t>(fluss::TypeId::Array);
- col.nullable = nullable;
- col.comment = rust::String("");
- col.precision = 0;
- col.scale = 0;
- col.array_nesting = nesting;
- if (!per_level_nullability.empty()) {
- for (auto v : per_level_nullability) {
- col.array_nullability.push_back(v);
- }
- } else {
- for (int32_t i = 0; i < nesting; ++i) {
- col.array_nullability.push_back((i == 0 ? nullable : true) ? 1 :
0);
- }
- col.array_nullability.push_back(leaf_nullable ? 1 : 0);
- }
- col.element_data_type = element_type;
- col.element_precision = 0;
- col.element_scale = 0;
- return col;
-}
-
-fluss::ffi::FfiColumn MakeScalarColumn(const char* name, fluss::TypeId type_id,
- bool nullable = true, int32_t precision
= 0,
- int32_t scale = 0) {
- fluss::ffi::FfiColumn col;
- col.name = rust::String(name);
- col.data_type = static_cast<int32_t>(type_id);
- col.nullable = nullable;
- col.comment = rust::String("");
- col.precision = precision;
- col.scale = scale;
- col.array_nesting = 0;
- col.element_data_type = 0;
- col.element_precision = 0;
- col.element_scale = 0;
- return col;
-}
-
-} // namespace
+using fluss::Column;
+using fluss::DataType;
+using fluss::TypeId;
-TEST(FfiConverterTest, RejectsArrayWithoutElementType) {
- auto col = MakeArrayColumn(1, 0);
- EXPECT_THROW((void)fluss::utils::from_ffi_column(col), std::runtime_error);
+// Encode a column to the FFI node arena and decode it back, exercising the
+// full create-table / get-table-info type transport.
+Column RoundTrip(const Column& col) {
Review Comment:
Nit: since the targets are precision, comment and char, can we add an
integration test that create a table with those type, and then assert
get-table-info returns exactly those values?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
