leekeiabstraction commented on code in PR #288:
URL: https://github.com/apache/fluss-rust/pull/288#discussion_r2779753708
##########
bindings/cpp/src/types.rs:
##########
@@ -430,99 +413,106 @@ fn core_row_to_ffi_fields(
for (i, field) in schema.fields().iter().enumerate() {
if row.is_null_at(i) {
- fields.push(new_datum(DATUM_TYPE_NULL));
+ fields.push(ffi::FfiDatum::default());
continue;
}
let datum = match field.data_type() {
- ArrowDataType::Boolean => {
- let mut datum = new_datum(DATUM_TYPE_BOOL);
- datum.bool_val = row.get_boolean(i);
- datum
- }
- ArrowDataType::Int8 => {
- let mut datum = new_datum(DATUM_TYPE_INT32);
- datum.i32_val = row.get_byte(i) as i32;
- datum
- }
- ArrowDataType::Int16 => {
- let mut datum = new_datum(DATUM_TYPE_INT32);
- datum.i32_val = row.get_short(i) as i32;
- datum
- }
- ArrowDataType::Int32 => {
- let mut datum = new_datum(DATUM_TYPE_INT32);
- datum.i32_val = row.get_int(i);
- datum
- }
- ArrowDataType::Int64 => {
- let mut datum = new_datum(DATUM_TYPE_INT64);
- datum.i64_val = row.get_long(i);
- datum
- }
- ArrowDataType::Float32 => {
- let mut datum = new_datum(DATUM_TYPE_FLOAT32);
- datum.f32_val = row.get_float(i);
- datum
- }
- ArrowDataType::Float64 => {
- let mut datum = new_datum(DATUM_TYPE_FLOAT64);
- datum.f64_val = row.get_double(i);
- datum
- }
- ArrowDataType::Utf8 => {
- let mut datum = new_datum(DATUM_TYPE_STRING);
- // todo: avoid copy string
- datum.string_val = row.get_string(i).to_string();
- datum
- }
+ ArrowDataType::Boolean => ffi::FfiDatum {
+ datum_type: DATUM_TYPE_BOOL,
+ bool_val: row.get_boolean(i),
+ ..Default::default()
+ },
+ ArrowDataType::Int8 => ffi::FfiDatum {
+ datum_type: DATUM_TYPE_INT32,
+ i32_val: row.get_byte(i) as i32,
+ ..Default::default()
+ },
+ ArrowDataType::Int16 => ffi::FfiDatum {
+ datum_type: DATUM_TYPE_INT32,
+ i32_val: row.get_short(i) as i32,
+ ..Default::default()
+ },
Review Comment:
Q for my understanding, it seems like we do not have INT16 and INT8 in Cpp
side. What happens if user attempts to upsert/append a row with value larger
than i16:MAX on a column that is of the type i16 on rust/java side?
Do they run into exception early or late?
##########
bindings/cpp/src/types.rs:
##########
@@ -646,6 +646,151 @@ fn core_row_to_ffi_fields(
fields
}
+impl Default for ffi::FfiDatum {
+ fn default() -> Self {
+ Self {
+ datum_type: DATUM_TYPE_NULL,
+ bool_val: false,
+ i32_val: 0,
+ i64_val: 0,
+ f32_val: 0.0,
+ f64_val: 0.0,
+ string_val: String::new(),
+ bytes_val: vec![],
+ decimal_precision: 0,
+ decimal_scale: 0,
+ i128_hi: 0,
+ i128_lo: 0,
+ }
+ }
+}
+
+/// Convert any InternalRow to FfiGenericRow using Fluss schema metadata.
+/// Used for lookup results (CompactedRow) where Arrow schema is unavailable.
+pub fn internal_row_to_ffi_row(
+ row: &dyn fcore::row::InternalRow,
+ table_info: &fcore::metadata::TableInfo,
+) -> Result<ffi::FfiGenericRow> {
+ let schema = table_info.get_schema();
+ let columns = schema.columns();
+ let mut fields = Vec::with_capacity(columns.len());
+
+ for (i, col) in columns.iter().enumerate() {
+ if row.is_null_at(i) {
+ fields.push(ffi::FfiDatum::default());
+ continue;
+ }
+
+ let datum = match col.data_type() {
+ fcore::metadata::DataType::Boolean(_) => ffi::FfiDatum {
+ datum_type: DATUM_TYPE_BOOL,
+ bool_val: row.get_boolean(i),
+ ..Default::default()
+ },
+ fcore::metadata::DataType::TinyInt(_) => ffi::FfiDatum {
+ datum_type: DATUM_TYPE_INT32,
+ i32_val: row.get_byte(i) as i32,
+ ..Default::default()
+ },
+ fcore::metadata::DataType::SmallInt(_) => ffi::FfiDatum {
+ datum_type: DATUM_TYPE_INT32,
+ i32_val: row.get_short(i) as i32,
+ ..Default::default()
+ },
Review Comment:
Similar q as above
##########
bindings/cpp/include/fluss.hpp:
##########
@@ -833,7 +898,13 @@ class Table {
bool Available() const;
+ GenericRow NewRow() const;
+
Result NewAppendWriter(AppendWriter& out);
+ Result NewUpsertWriter(UpsertWriter& out);
+ Result NewUpsertWriter(UpsertWriter& out, const std::vector<std::string>&
column_names);
+ Result NewUpsertWriter(UpsertWriter& out, const std::vector<size_t>&
column_indices);
Review Comment:
Appreciate that you're following current Cpp convention for NewAppendWriter.
However, should we consider following Java/Rust side closer? i.e. Table returns
TableUpsert, TableUpsert returns partial update based on supplied targetColumns
as going into v 0.1.0 is a one way door.
--
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]
