-
Notifications
You must be signed in to change notification settings - Fork 141
fix(ibis): Fix the incorrect data type in the test case #1288
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
WalkthroughUpdated a Redshift test to expect specific Redshift dtype strings in the IAM credential path, aligning them with the non-IAM test. No data or control-flow changes; only assertions on dtypes were modified. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Suggested labels
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
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: 1
🧹 Nitpick comments (2)
ibis-server/tests/routers/v2/connector/test_redshift.py (2)
160-162: Nit: Manifest declares totalprice as float while test asserts decimal128(5, 2)If the backend intentionally coerces totalprice to a fixed-precision Decimal, consider either:
- adjusting the manifest column type comment to reflect the coercion, or
- leaving a short note in the test for future readers.
This helps reconcile the apparent mismatch between manifest type and asserted Arrow dtype.
145-168: DRY up expected row and dtype maps shared by both testsBoth query tests assert the same row payload and dtype mapping. Extracting shared constants reduces duplication and future drift.
Apply this diff within this test to reference shared constants:
@@ - assert result["data"][0] == [ - 1, - 655, - "O", - "347.61", - "2023-05-21", - "1_655", - "2024-01-01 23:59:59.000000", - "2024-01-01 23:59:59.000000 +00:00", - None, - "abc", - ] - assert result["dtypes"] == { - "orderkey": "int64", - "custkey": "int64", - "orderstatus": "string", - "totalprice": "decimal128(5, 2)", - "orderdate": "date32[day]", - "order_cust_key": "string", - "timestamp": "timestamp[ns]", - "timestamptz": "timestamp[ns, tz=UTC]", - "test_null_time": "null", - "bytea_column": "string", - } + assert result["data"][0] == EXPECTED_ROW + assert result["dtypes"] == EXPECTED_DTYPESAnd add these shared constants near the top of the file (outside the selected range), reusing them in both tests:
# Shared expected payloads across Redshift query tests EXPECTED_ROW = [ 1, 655, "O", "347.61", "2023-05-21", "1_655", "2024-01-01 23:59:59.000000", "2024-01-01 23:59:59.000000 +00:00", None, "abc", ] EXPECTED_DTYPES = { "orderkey": "int64", "custkey": "int64", "orderstatus": "string", "totalprice": "decimal128(5, 2)", "orderdate": "date32[day]", "order_cust_key": "string", "timestamp": "timestamp[ns]", "timestamptz": "timestamp[ns, tz=UTC]", "test_null_time": "null", "bytea_column": "string", }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
ibis-server/tests/routers/v2/connector/test_redshift.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: ci
🔇 Additional comments (1)
ibis-server/tests/routers/v2/connector/test_redshift.py (1)
160-167: LGTM: IAM path dtype expectations updated to Arrow types to match non-IAM testThis brings the IAM-credential flow in line with the standard connection test and avoids false negatives after the Arrow switch. Looks good.
Since we changed to use Arrow as our underlying data type system, fixed this test case.
Summary by CodeRabbit