Skip to content

[client] Added unit test cases#310

Open
niknegi wants to merge 1 commit intoapache:mainfrom
niknegi:unit_test
Open

[client] Added unit test cases#310
niknegi wants to merge 1 commit intoapache:mainfrom
niknegi:unit_test

Conversation

@niknegi
Copy link

@niknegi niknegi commented Feb 13, 2026

Purpose

Linked issue: #309

Brief change log

Add unit tests for DataType validation edge cases and RowType projection

Tests

Screenshot 2026-02-13 at 12 08 54 PM

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This pull request adds comprehensive unit tests for DataType validation edge cases and RowType projection functionality, addressing issue #309. The changes introduce test coverage for precision/scale validation in DecimalType, TimeType, and TimestampType, as well as tests for RowType field projection operations.

Changes:

  • Added is_nullable() methods to DecimalType, TimeType, TimestampType, and TimestampLTzType
  • Added unit tests for DecimalType validation covering valid and invalid precision/scale combinations
  • Added unit tests for TimeType and TimestampType valid precision ranges (0-9)
  • Added unit tests for RowType projection by indices and field names, including error cases

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Contributor

@luoyuxia luoyuxia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@niknegi Thanks for the pr. Left minor comments. PTAL


// Mixed existing and non-existing: only existing fields are projected
let projected = row_type
.project_with_field_names(&["id".to_string(), "nonexistent".to_string()])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd argue it should throw exceptioin when the field doesn't exist just like java side. Could you please help fix it?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, checking it!

Copy link
Contributor

@luoyuxia luoyuxia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@niknegi Thanks for update. LGTM. But ci fails

@niknegi
Copy link
Author

niknegi commented Feb 14, 2026

@niknegi Thanks for update. LGTM. But ci fails

Fixed the formatting which caused the ci to fail

@niknegi niknegi requested a review from luoyuxia February 14, 2026 12:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants