-
Notifications
You must be signed in to change notification settings - Fork 132
improve error messages in func when complex column passed #1441
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
Conversation
Reviewer's GuideThis PR introduces strict guards in SQL function handling to disallow Pydantic-based complex object columns with clear error messages, adds a fallback resolution path for unresolved columns, and includes targeted tests to ensure these safeguards in aggregate, conditional, and unit contexts. Sequence diagram for error handling when complex column is passed to SQL functionsequenceDiagram
participant User
participant Func
participant SignalSchema
participant ModelStore
participant Error
User->>Func: Call SQL function with column
Func->>SignalSchema: get_column_type(arg, with_subtree=True)
SignalSchema-->>Func: Return column type
Func->>ModelStore: is_pydantic(column_type)
ModelStore-->>Func: Return True (if complex)
Func->>Error: Raise DataChainParamsError with message
Error-->>User: Error message: "Function X doesn't support complex object columns..."
Class diagram for updated error handling in func.pyclassDiagram
class Func {
+get_column(signals_schema, label, table)
+_db_cols
+name
}
class SignalSchema {
+get_column_type(name, with_subtree)
}
class ModelStore {
+is_pydantic(type)
}
class DataChainParamsError
Func --> SignalSchema : uses
Func --> ModelStore : uses
Func --> DataChainParamsError : raises
Class diagram for updated get_db_col_type fallback logicclassDiagram
class Func {
+get_db_col_type(signals_schema, col)
}
class SignalSchema {
+get_column_type(name)
+get_column_type(name, with_subtree)
}
class SignalResolvingError
Func --> SignalSchema : calls
Func --> SignalResolvingError : handles
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey there - I've reviewed your changes - here's some feedback:
- Consider extracting the complex‐object guard logic into a shared helper so you can reuse it in both
get_columnandget_db_col_typeand keep the error messaging consistent. - The
SignalResolvingErrorimport insideget_db_col_typecould be moved to the module level, and you may want to narrow or log the fallback catch so it doesn’t silently mask other schema resolution issues. - It might help to centralize the error message text (e.g. in a constant) to ensure that all SQL functions report the same actionable guidance when a complex column is passed.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider extracting the complex‐object guard logic into a shared helper so you can reuse it in both `get_column` and `get_db_col_type` and keep the error messaging consistent.
- The `SignalResolvingError` import inside `get_db_col_type` could be moved to the module level, and you may want to narrow or log the fallback catch so it doesn’t silently mask other schema resolution issues.
- It might help to centralize the error message text (e.g. in a constant) to ensure that all SQL functions report the same actionable guidance when a complex column is passed.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
a18c0de to
2ce1776
Compare
Deploying datachain-documentation with
|
| Latest commit: |
9ee76ff
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://dee042d2.datachain-documentation.pages.dev |
| Branch Preview URL: | https://guard-complex-types-func.datachain-documentation.pages.dev |
2ce1776 to
f9a9c58
Compare
f9a9c58 to
9ee76ff
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1441 +/- ##
=======================================
Coverage 87.90% 87.91%
=======================================
Files 160 160
Lines 15300 15309 +9
Branches 2206 2210 +4
=======================================
+ Hits 13450 13459 +9
Misses 1336 1336
Partials 514 514
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
dreadatour
left a comment
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.
Looks good to me! 👍
Makes it a bit easier to understand what is going on in certain user scenarios.
Summary by Sourcery
Disallow complex object columns in SQL functions by raising descriptive errors and add fallback resolution for complex column types.
Enhancements:
Tests: