chore: rmreplace_columns, unused code
#1251
Conversation
WalkthroughThis pull request involves the removal of the Changes
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
de-sh
changed the title
replace_columnsreplace_columns, unused code
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/utils/arrow/mod.rs (1)
1-200: 🛠️ Refactor suggestionConsider documenting the removal of public API
The removal of
replace_columnsconstitutes a breaking change to the public API of this module. When removing public functions, it's a good practice to:
- Document the rationale for the removal
- Provide migration guidance for users of the function
- Consider versioning implications
Since this appears to be a refactoring rather than just a removal, please ensure there's appropriate documentation explaining how to achieve the same functionality with the new approach.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/event/format/mod.rs(0 hunks)src/utils/arrow/mod.rs(2 hunks)
💤 Files with no reviewable changes (1)
- src/event/format/mod.rs
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: coverage
- GitHub Check: Build Default x86_64-pc-windows-msvc
- GitHub Check: Build Default x86_64-apple-darwin
- GitHub Check: Build Default x86_64-unknown-linux-gnu
- GitHub Check: Build Default aarch64-unknown-linux-gnu
- GitHub Check: Build Default aarch64-apple-darwin
- GitHub Check: Build Kafka aarch64-apple-darwin
- GitHub Check: Build Kafka x86_64-unknown-linux-gnu
- GitHub Check: Quest Smoke and Load Tests for Standalone deployments
- GitHub Check: Quest Smoke and Load Tests for Distributed deployments
🔇 Additional comments (2)
src/utils/arrow/mod.rs (2)
25-25:❓ Verification inconclusive
Import changes might affect dependent code
The import was modified to remove
Arrayfrom thearrow_arrayimports. This indicates that theArraytype is no longer needed in this module, which is consistent with the removal of functionality that might have used it.This change seems reasonable, assuming all code that previously relied on this import has been addressed, but verify that related functionality in other files has been updated accordingly.
🏁 Script executed:
#!/bin/bash # Check if there are any remaining references to Array from arrow_array that might now be broken rg "arrow_array::Array" --type rustLength of output: 35
Verification Notice: Confirm no dependent usage of
arrow_array::ArrayremainsBased on an updated repository-wide search (using
rg "arrow_array::Array" --type rust), there were no hits indicating any remaining references toarrow_array::Array. This suggests that the removal insrc/utils/arrow/mod.rsis complete. However, because the search output was minimal, please manually verify that no dependent code relies on this import (or indirectly on the removed functionality).• Verify across the codebase that no grouped imports or indirect references are attempting to use
Arrayfromarrow_array.If manual confirmation or additional testing shows no negative impact, you can consider this import change acceptable.
99-150:✅ Verification successful
Verify that
add_parseable_fieldsstill works withoutreplace_columnsThe
add_parseable_fieldsfunction creates a new record batch with additional fields. According to the AI summary, there was a related change in another file where a line initializing therbvariable ininto_recordbatchmethod was removed.Please verify that after removing
replace_columns, theadd_parseable_fieldsfunction is still being called with properly initialized record batches throughout the codebase.
🏁 Script executed:
#!/bin/bash # Look for calls to add_parseable_fields to ensure they're providing initialized record batches rg "add_parseable_fields\(" -A 2 -B 2 --type rust # Check the into_recordbatch method to see how rb is being initialized now rg "into_recordbatch" -A 10 -B 10 --type rustLength of output: 16661
Confirm proper batch initialization after removing
replace_columnsThe investigation confirms that all calls to
add_parseable_fieldsare still receiving well-formed record batches. In particular:
- In src/event/format/mod.rs, the record batch generated by
Self::decodeis passed toadd_parseable_fieldswith all expected fields.- In src/handlers/http/ingest.rs, tests calling the
into_recordbatchmethod (which internally usesadd_parseable_fields) verify that the record batches have the correct schema, row count, and columns.No issues were found with the initialization process after the removal of
replace_columns.
#1218 (comment)
Fixes #XXXX.
Description
This PR has:
Summary by CodeRabbit
Summary by CodeRabbit
replace_columnsfunction and associated logic for column replacement in record batches.into_recordbatchmethod.