You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/04/10 04:42:11 UTC
[GitHub] [arrow-datafusion] yjshen opened a new pull request, #2189: Multiple row-layout support, part-1: Restructure code for clearness
yjshen opened a new pull request, #2189:
URL: https://github.com/apache/arrow-datafusion/pull/2189
# Which issue does this PR close?
The first part for #2188.
# Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
- JIT feature gate `#[cfg(feature = "jit")]` is scattered all over the code for row reader/writer.
- I'm trying to figure out how to use the existing cell getter/setters for different row layouts. Moving unrelated parts to their own module would make further refactoring easier.
# What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
Moving codes around for clearness.
# Are there any user-facing changes?
No
--
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
[GitHub] [arrow-datafusion] yjshen merged pull request #2189: Multiple row-layout support, part-1: Restructure code for clearness
Posted by GitBox <gi...@apache.org>.
yjshen merged PR #2189:
URL: https://github.com/apache/arrow-datafusion/pull/2189
--
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
[GitHub] [arrow-datafusion] yjshen commented on pull request #2189: Multiple row-layout support, part-1: Restructure code for clearness
Posted by GitBox <gi...@apache.org>.
yjshen commented on PR #2189:
URL: https://github.com/apache/arrow-datafusion/pull/2189#issuecomment-1094603714
I am merging this to unlock further row layout implementations. Thanks @alamb!
--
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
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #2189: Multiple row-layout support, part-1: Restructure code for clearness
Posted by 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
[GitHub] [arrow-datafusion] yjshen commented on a diff in pull request #2189: Multiple row-layout support, part-1: Restructure code for clearness
Posted by GitBox <gi...@apache.org>.
yjshen commented on code in PR #2189:
URL: https://github.com/apache/arrow-datafusion/pull/2189#discussion_r846773771
##########
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:
The main reason for this change is to support the use case in sort payload output, where we need to chase compositeIndex pointers and output rows that belongs to different input batches/pages. So we could therefore point to a record, append its cell to output record batch buffer, and ponit to the next record.
Since it's just a field assignment without expensive calculations, I think it's acceptable here.
--
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
[GitHub] [arrow-datafusion] yjshen commented on a diff in pull request #2189: Multiple row-layout support, part-1: Restructure code for clearness
Posted by GitBox <gi...@apache.org>.
yjshen commented on code in PR #2189:
URL: https://github.com/apache/arrow-datafusion/pull/2189#discussion_r846774387
##########
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:
Yes, RowReader and RowWriter is meant to be used outside this row module. the underneath implementation could be chosen based on whether the jit feature gate is enabled I think.
--
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