Another round of fixes for null columns handling #3103
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses issues related to handling NULL values in numeric columns within a DeepLake PostgreSQL extension. The changes ensure that NULL numeric values are consistently stored as their type's default (typically 0) rather than as undefined values, which resolves problems with schema changes and data operations.
Key Changes:
- Added a helper function to map NULL numeric values to appropriate default values (0 for numeric types, false for boolean)
- Modified batch evaluation logic to handle arrays containing NULL values by converting them to default values
- Added comprehensive test coverage for NULL handling in numeric columns
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| postgres/tests/py_tests/test_add_numeric_columns.py | New test file verifying NULL handling in numeric columns during table creation, column addition, inserts, and updates |
| cpp/deeplake_pg/table_storage.cpp | Added get_default_value_for_null() helper and integrated it into the NULL value conversion logic |
| cpp/deeplake_pg/table_data_impl.hpp | Updated batch initialization to use eval_with_nones<T>() for proper NULL handling |
| cpp/deeplake_pg/nd_utils.hpp | Added eval_with_nones<T>() template function to evaluate arrays containing NULL values |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| """) | ||
| assert len(rows) == 0, f"Expected 0 rows after adding column, got {len(rows)}" | ||
|
|
||
| # Insert row with empty name and NULL UUID (numeric column) |
There was a problem hiding this comment.
The comment incorrectly refers to 'UUID' when the column is actually NUMERIC. The comment should say 'NULL numeric value' instead of 'NULL UUID (numeric column)'.
| # Insert row with empty name and NULL UUID (numeric column) | |
| # Insert row with empty name and NULL numeric value |
| { | ||
| try { | ||
| return nd::eval(arr); | ||
| } catch (const nd::invalid_dynamic_eval&) { |
There was a problem hiding this comment.
Empty catch block silently swallows the exception without any logging or documentation explaining why this exception is expected and safe to ignore. Consider adding a comment explaining the expected condition or logging at debug level.
| } catch (const nd::invalid_dynamic_eval&) { | |
| } catch (const nd::invalid_dynamic_eval&) { | |
| // nd::eval failed due to an invalid dynamic evaluation (e.g. incompatible shapes). | |
| // This is expected for some inputs; fall back to element-wise evaluation with None handling below. |
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
This function lacks documentation explaining its purpose, parameters, return value, and when it should be used instead of plain nd::eval. Add a docstring explaining that this function evaluates arrays containing NULL values by replacing them with default-constructed values of type T.
| /** | |
| * Evaluates a dynamic nd::array that may contain NULL (none) values by | |
| * replacing those NULLs with default-constructed values of type T. | |
| * | |
| * This helper first attempts to evaluate the input array with nd::eval. If | |
| * that succeeds, the result is returned directly. If nd::eval throws | |
| * nd::invalid_dynamic_eval (for example, because the presence of none values | |
| * prevents dynamic evaluation), the array is traversed and any element that | |
| * is none is replaced with nd::adapt(T()), after which nd::eval is applied | |
| * to the reconstructed array. | |
| * | |
| * Use this function instead of calling nd::eval directly when the input | |
| * array may legitimately contain NULL values and you want those values to | |
| * be treated as default-constructed instances of T during evaluation. | |
| * | |
| * @tparam T The value type used to replace NULL (none) elements. | |
| * @param arr A dynamic nd::array that may contain none elements. | |
| * @return The result of evaluating the array after substituting none | |
| * elements with default-constructed values of type T when needed. | |
| */ |
|
🚀 🚀 Pull Request
Impact
Description
Things to be aware of
Things to worry about
Additional Context