-
Notifications
You must be signed in to change notification settings - Fork 11
Allows None as column value in Sender row #116
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
WalkthroughThe pull request expands type hints in the QuestDB ingress module to explicitly allow Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/questdb/ingress.pyx (1)
2536-2536: Type hint correctly updated to allow None values.The addition of
Noneto the Union aligns theSender.rowtype signature withBuffer.rowandSenderTransaction.row, accurately reflecting the existing runtime behavior (lines 1122-1125 skip None values).Optional future improvement: Consider also including
TimestampNanosandDecimalin the type hint. The runtime already supports both (_columnmethod at lines 1039-1040 forTimestampNanosand lines 1045-1046 forDecimal), and they're present in similar type hints forBuffer.row(line 1144) andSenderTransaction.row(line 654). This would improve type safety and consistency across the API.src/questdb/ingress.pyi (1)
1036-1036: Type hint correctly updated, but consider adding missing types for consistency.The addition of
Noneto the Union is correct and aligns with the runtime behavior. However, there's an inconsistency in this type stub file:
Buffer.row(line 386) andSenderTransaction.row(line 207) includeDecimalin their column type unionsSender.row(line 1036) does NOT includeDecimalorTimestampNanosSince all three methods ultimately use the same serialization logic that supports
DecimalandTimestampNanos, the type hints should be consistent.Consider updating the type hint to:
Dict[str, Union[None, bool, int, float, str, TimestampMicros, TimestampNanos, datetime, np.ndarray, Decimal]]This would match the capabilities in
Buffer.rowandSenderTransaction.rowand accurately reflect the runtime support in the_columnmethod (lines 1026-1057 in ingress.pyx).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/questdb/ingress.pyi(1 hunks)src/questdb/ingress.pyx(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-22T07:31:37.016Z
Learnt from: RaphDal
Repo: questdb/py-questdb-client PR: 114
File: src/questdb/ingress.pyx:192-195
Timestamp: 2025-10-22T07:31:37.016Z
Learning: In src/questdb/ingress.pyx, the IngressErrorCode.BadDataFrame enum member is Python-internal only and does not map to any C error code from the underlying library. It is defined as `<int>line_sender_error_invalid_decimal + 1` and should not be added to the c_err_code_to_py function, which only handles C-to-Python error code translation.
Applied to files:
src/questdb/ingress.pyxsrc/questdb/ingress.pyi
amunra
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.
Thank you!
I noticed that Buffer and SenderTransaction classes allows None columns values
However, the exposed Sender class does not allow this.
I just added a type hint to allow it.
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.