-
Notifications
You must be signed in to change notification settings - Fork 21
[ISSUE-149] Add column projection support to Python LogScanner #151
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -70,9 +70,112 @@ impl FlussTable { | |
| let table_scan = fluss_table.new_scan(); | ||
|
|
||
| let rust_scanner = table_scan.create_log_scanner().map_err(|e| { | ||
| PyErr::new::<pyo3::exceptions::PyRuntimeError, _>(format!( | ||
| "Failed to create log scanner: {e:?}" | ||
| )) | ||
| FlussError::new_err(format!("Failed to create log scanner: {e}")) | ||
| })?; | ||
|
|
||
| let admin = conn | ||
| .get_admin() | ||
| .await | ||
| .map_err(|e| FlussError::new_err(e.to_string()))?; | ||
|
|
||
| let py_scanner = LogScanner::from_core(rust_scanner, admin, table_info.clone()); | ||
| Python::attach(|py| Py::new(py, py_scanner)) | ||
| }) | ||
| } | ||
|
|
||
| /// Create a new log scanner with column projection (by index). | ||
| /// | ||
| /// Args: | ||
| /// column_indices: List of column indices to include in the scan (0-based) | ||
| /// | ||
| /// Returns: | ||
| /// LogScanner with projection applied | ||
| /// | ||
| /// Example: | ||
| /// >>> scanner = await table.new_log_scanner_with_projection([0, 2, 4]) | ||
| pub fn new_log_scanner_with_projection<'py>( | ||
| &self, | ||
| py: Python<'py>, | ||
| column_indices: Vec<usize>, | ||
| ) -> PyResult<Bound<'py, PyAny>> { | ||
| // Validate early with Python-friendly error | ||
| if column_indices.is_empty() { | ||
| return Err(FlussError::new_err( | ||
| "column_indices cannot be empty".to_string(), | ||
| )); | ||
| } | ||
|
|
||
| let conn = self.connection.clone(); | ||
| let metadata = self.metadata.clone(); | ||
| let table_info = self.table_info.clone(); | ||
|
|
||
| future_into_py(py, async move { | ||
| let fluss_table = | ||
| fcore::client::FlussTable::new(&conn, metadata.clone(), table_info.clone()); | ||
|
|
||
| let table_scan = fluss_table.new_scan(); | ||
| let table_scan = table_scan | ||
| .project(&column_indices) | ||
| .map_err(|e| FlussError::new_err(format!("Failed to project columns: {e}")))?; | ||
|
|
||
| let rust_scanner = table_scan.create_log_scanner().map_err(|e| { | ||
| FlussError::new_err(format!("Failed to create log scanner: {e}")) | ||
| })?; | ||
|
Comment on lines
+121
to
+123
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We seem to use FlussError and PyErr within this file, for example line 72 to 75 uses PyErr. Can you clarify when each should be used?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we should use FlussError, this is a leftover. Thank you for catching this 👍 |
||
|
|
||
| let admin = conn | ||
| .get_admin() | ||
| .await | ||
| .map_err(|e| FlussError::new_err(e.to_string()))?; | ||
|
|
||
| let py_scanner = LogScanner::from_core(rust_scanner, admin, table_info.clone()); | ||
| Python::attach(|py| Py::new(py, py_scanner)) | ||
| }) | ||
| } | ||
|
|
||
| /// Create a new log scanner with column projection (by name). | ||
| /// | ||
| /// This is the more Pythonic way to specify projections using column names | ||
| /// instead of indices. | ||
| /// | ||
| /// Args: | ||
| /// column_names: List of column names to include in the scan | ||
| /// | ||
| /// Returns: | ||
| /// LogScanner with projection applied | ||
| /// | ||
| /// Example: | ||
| /// >>> scanner = await table.new_log_scanner_with_column_names(["id", "name", "email"]) | ||
| pub fn new_log_scanner_with_column_names<'py>( | ||
| &self, | ||
| py: Python<'py>, | ||
| column_names: Vec<String>, | ||
| ) -> PyResult<Bound<'py, PyAny>> { | ||
| // Validate early with Python-friendly error | ||
| if column_names.is_empty() { | ||
| return Err(FlussError::new_err( | ||
| "column_names cannot be empty".to_string(), | ||
| )); | ||
| } | ||
|
|
||
| let conn = self.connection.clone(); | ||
| let metadata = self.metadata.clone(); | ||
| let table_info = self.table_info.clone(); | ||
|
|
||
| future_into_py(py, async move { | ||
| let fluss_table = | ||
| fcore::client::FlussTable::new(&conn, metadata.clone(), table_info.clone()); | ||
|
|
||
| // Convert Vec<String> to Vec<&str> for the API | ||
| // Safe: project_by_name validates names immediately, doesn't store refs | ||
| let column_name_refs: Vec<&str> = column_names.iter().map(|s| s.as_str()).collect(); | ||
|
|
||
| let table_scan = fluss_table.new_scan(); | ||
| let table_scan = table_scan | ||
| .project_by_name(&column_name_refs) | ||
| .map_err(|e| FlussError::new_err(format!("Failed to project columns: {e}")))?; | ||
|
|
||
| let rust_scanner = table_scan.create_log_scanner().map_err(|e| { | ||
| FlussError::new_err(format!("Failed to create log scanner: {e}")) | ||
| })?; | ||
|
|
||
| let admin = conn | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the polling part also be included (as with C++ example)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I get what you mean.
to_pandas()polls internally, but if we're talking about adding a separate polling API to Python bindings - we can add it.Let's file an issue for it, as it's orthogonal to the column projection feature.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created an issue #152
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I didn't realise that to panda polls. Thank you for the clarification