[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #2189: Multiple row-layout support, part-1: Restructure code for clearness

[GitBox <gi...@apache.org>]

alamb commented on code in PR #2189:
URL: https://github.com/apache/arrow-datafusion/pull/2189#discussion_r846769772


##########
datafusion/core/src/row/reader.rs:
##########
@@ -18,71 +18,33 @@
 //! Accessing row from raw bytes
 
 use crate::error::{DataFusionError, Result};
-#[cfg(feature = "jit")]
-use crate::reg_fn;
-#[cfg(feature = "jit")]
-use crate::row::fn_name;
-use crate::row::{
-    all_valid, get_offsets, schema_null_free, supported, NullBitsFormatter,
-};
+use crate::row::layout::get_offsets;
+use crate::row::validity::{all_valid, NullBitsFormatter};
+use crate::row::{row_supported, schema_null_free, MutableRecordBatch};
 use arrow::array::*;
 use arrow::datatypes::{DataType, Schema};
-use arrow::error::Result as ArrowResult;
 use arrow::record_batch::RecordBatch;
 use arrow::util::bit_util::{ceil, get_bit_raw};
-#[cfg(feature = "jit")]
-use datafusion_jit::api::Assembler;
-#[cfg(feature = "jit")]
-use datafusion_jit::api::GeneratedFunction;
-#[cfg(feature = "jit")]
-use datafusion_jit::ast::{I64, PTR};
 use std::sync::Arc;
 
 /// Read `data` of raw-bytes rows starting at `offsets` out to a record batch
 pub fn read_as_batch(
     data: &[u8],
     schema: Arc<Schema>,
-    offsets: Vec<usize>,
+    offsets: &[usize],
 ) -> Result<RecordBatch> {
     let row_num = offsets.len();
     let mut output = MutableRecordBatch::new(row_num, schema.clone());
-    let mut row = RowReader::new(&schema, data);
+    let mut row = RowReader::new(&schema);
 
     for offset in offsets.iter().take(row_num) {
-        row.point_to(*offset);
+        row.point_to(*offset, data);

Review Comment:
   it seems strange to update `row.data` on each new offset as `data` isn't changing from iteration to iteration here



##########
datafusion/core/src/row/jit/reader.rs:
##########
@@ -0,0 +1,164 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Accessing row from raw bytes with JIT
+
+use crate::error::{DataFusionError, Result};
+use crate::reg_fn;
+use crate::row::jit::fn_name;
+use crate::row::reader::RowReader;
+use crate::row::reader::*;
+use crate::row::MutableRecordBatch;
+use arrow::array::ArrayBuilder;
+use arrow::datatypes::{DataType, Schema};
+use arrow::record_batch::RecordBatch;
+use datafusion_jit::api::Assembler;
+use datafusion_jit::api::GeneratedFunction;
+use datafusion_jit::ast::{I64, PTR};
+use std::sync::Arc;
+
+/// Read `data` of raw-bytes rows starting at `offsets` out to a record batch
+
+pub fn read_as_batch_jit(

Review Comment:
   I wonder if over the long term we can hide all the reading/write as jit / not as jit within the RowReader / RowWriter -- so that most code in DataFusion will simply use RowReader/RowWriter and the use of jit would be an implementation detail
   
   This may be where you are headed anyways, I just wanted to say it explicitly



##########
datafusion/core/src/row/reader.rs:
##########
@@ -18,71 +18,33 @@
 //! Accessing row from raw bytes
 
 use crate::error::{DataFusionError, Result};
-#[cfg(feature = "jit")]

Review Comment:
   ❤️ 



-- 
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-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org