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.

Sign up for GitHub

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

Fix typo of calculating offsets for slicing index #206

Merged
merged 1 commit into from
Sep 27, 2022
Merged

Fix typo of calculating offsets for slicing index #206

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions cpp/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ include_directories(src)
include_directories(${CMAKE_BINARY_DIR}/src) # for format.pb.{h/cc}

function(add_lance_test test_name)
if (NOT CMAKE_BUILD_TYPE STREQUAL "Release")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change CMakeFile to allow python side to build ReleaseWithDebugInfo mode , easier to see call stack.

if (CMAKE_BUILD_TYPE STREQUAL Debug)
add_executable(${test_name} ${test_name}.cc)
target_link_libraries(${test_name}
Catch2::Catch2WithMain
Expand All @@ -105,7 +105,7 @@ function(add_lance_test test_name)
endif ()
endfunction()

if (NOT CMAKE_BUILD_TYPE STREQUAL "Release")
if (CMAKE_BUILD_TYPE STREQUAL Debug)
include(CTest)
include(Catch)
set(test_libs Catch2::Catch2WithMain $<TARGET_OBJECTS:lance_testing>)
Expand Down
2 changes: 1 addition & 1 deletion cpp/src/lance/io/exec/base.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ ScanBatch ScanBatch::Slice(int64_t off, int64_t length) const {
auto sliced_batch = batch->Slice(off, length);
decltype(indices) sliced_indices;
if (indices) {
sliced_indices = std::dynamic_pointer_cast<::arrow::Int32Array>(indices->Slice(offset, length));
sliced_indices = std::dynamic_pointer_cast<::arrow::Int32Array>(indices->Slice(off, length));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not a blocker for this PR but should we go with the convention to add an _ suffix to member variables? Would that have made it clearer here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This ScanBatch struct was a POD struct in the beginning, so that the caller can directly use scan_batch.batch to access its fields. It becomes more complicated lately, we could change it to class and wrap the fields as private and with _ suffix later?

I am fine to work on it after the release.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added #207 to follow up with the remaining concerns.

}
return ScanBatch(sliced_batch, batch_id, offset, sliced_indices);
}
Expand Down