Skip to content

[sc-42217] Update Numeric Types: Add int64 and string support for AttributeTypeInt64#90

Closed
Saurabhdarekar wants to merge 10 commits intoSGNL-ai:mainfrom
Saurabhdarekar:sc-42217-Update-Numeric-Types
Closed

[sc-42217] Update Numeric Types: Add int64 and string support for AttributeTypeInt64#90
Saurabhdarekar wants to merge 10 commits intoSGNL-ai:mainfrom
Saurabhdarekar:sc-42217-Update-Numeric-Types

Conversation

@Saurabhdarekar
Copy link
Copy Markdown
Contributor

@Saurabhdarekar Saurabhdarekar commented Aug 13, 2025

The framework.AttributeTypeInt64 handler in web/json_value.go only accepted float64 values, causing failures when developers provided int64 or string values directly. This forced adapter developers to manually cast values to float64 before passing them to the framework - a poor developer experience that also introduced precision loss for large integers.

Example of the issue:
// This would fail with the old implementation
obj.WithAttribute("userId", int64(123456789012345)) // Error!

// Developers had to do this workaround:
obj.WithAttribute("userId", float64(myInt64Value)) // Ugly + precision loss

Solution

Enhanced the AttributeTypeInt64 case to handle three input types:

  1. int64 - Direct return (no conversion needed)
  2. float64 - Convert with precision validation
  3. string - Parse using strconv.ParseInt

Key Improvements

🛡️ Precision Safety: Added validation for float64 → int64 conversions

  • Rejects values outside the safe integer range (±2^53-1)
  • Prevents fractional values from being silently truncated
  • Provides clear error messages when precision would be lost

🚀 Expanded Range: String parsing supports the full int64 range without precision loss

  • Handles values from -9223372036854775808 to 9223372036854775807
  • No more artificial limitations due to float64 precision boundaries

✨ Better Developer Experience:

  • Developers can provide int64 values directly without manual casting
  • Clear, actionable error messages guide developers when issues occur
  • Backward compatible with existing float64 inputs

Testing

  • ✅ All existing tests continue to pass
  • ✅ Added comprehensive test coverage for new int64 handling scenarios
  • ✅ Added tests for precision validation and error cases
  • ✅ Added integration tests in JSON object processing context

Breaking Changes

⚠️ Intentional safety improvements that may surface previously silent issues:

  • Values with fractional parts now error instead of truncating silently
  • Large integers beyond safe precision now error instead of corrupting data
  • These are good breaks that improve data integrity

@Saurabhdarekar Saurabhdarekar requested a review from a team as a code owner August 13, 2025 19:38
@Saurabhdarekar Saurabhdarekar changed the title Update Numeric Types: Add int64 and string support for AttributeTypeInt64 [sc-42217] Update Numeric Types: Add int64 and string support for AttributeTypeInt64 Aug 13, 2025
Copy link
Copy Markdown
Contributor

@nholbrook nholbrook left a comment

Choose a reason for hiding this comment

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

Overall good direction, just a few comments

Comment thread web/json_value.go Outdated
Comment thread web/json_value.go
Comment thread web/json_value.go
@codecov
Copy link
Copy Markdown

codecov Bot commented Aug 19, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 50.16%. Comparing base (6a6fbdd) to head (3f8a4f1).

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #90      +/-   ##
==========================================
+ Coverage   48.68%   50.16%   +1.48%     
==========================================
  Files          24       24              
  Lines        2654     2675      +21     
==========================================
+ Hits         1292     1342      +50     
+ Misses       1306     1287      -19     
+ Partials       56       46      -10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Contributor

@nholbrook nholbrook left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the previous comments, a few small remaining nits I noticed with the last round of changes. LGTM after these are addressed

Comment thread web/json_value.go Outdated
Comment thread web/json_value.go Outdated
Comment thread web/json_value.go Outdated
Comment thread web/json_object_test.go Outdated
Saurabhdarekar and others added 4 commits August 20, 2025 15:05
Co-authored-by: Nick Holbrook <ndh175@gmail.com>
Co-authored-by: Nick Holbrook <ndh175@gmail.com>
Co-authored-by: Nick Holbrook <ndh175@gmail.com>
nholbrook
nholbrook previously approved these changes Aug 21, 2025
Copy link
Copy Markdown
Contributor

@nholbrook nholbrook left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

As a followup (either in this story or a separate story), we should also look at updating instances in our adapters now that we can provide Int64 directly (e.g. https://github.com/SGNL-ai/adapters/blob/c628f27068bede43af3bc43e60b521e63d5afcfa/pkg/my-sql/datasource.go#L233-L236).

Copilot AI review requested due to automatic review settings September 3, 2025 17:32
Copy link
Copy Markdown

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 PR enhances the AttributeTypeInt64 handler to accept int64, float64, and string values instead of only float64, improving developer experience and preventing precision loss for large integers.

  • Enhanced int64 attribute handling to support direct int64 values, strings, and safe float64 conversions
  • Added comprehensive precision validation for float64 to int64 conversions to prevent data corruption
  • Extended double attribute type to also accept string values for consistency

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
web/json_value.go Enhanced AttributeTypeInt64 and AttributeTypeDouble cases to handle multiple input types with validation
web/json_value_test.go Added comprehensive test cases for new int64 and double string parsing scenarios
web/json_object_test.go Added integration tests for int64 support in JSON object processing context
.github/workflows/codeql.yml Added CodeQL security analysis workflow configuration

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment thread web/json_value.go
Comment thread web/json_value.go
@Saurabhdarekar
Copy link
Copy Markdown
Contributor Author

This PR is closed and a new PR is created for it to resolve the codeQL issue.
New PR: #93

Saurabhdarekar added a commit that referenced this pull request Sep 4, 2025
…nt64 (#93)

This PR is a copy of original
#90. I have migrated to
a new PR as for old PR codeQL workflow was not working.

The framework.AttributeTypeInt64 handler in web/json_value.go only
accepted float64 values, causing failures when developers provided int64
or string values directly. This forced adapter developers to manually
cast values to float64 before passing them to the framework - a poor
developer experience that also introduced precision loss for large
integers.

  Example of the issue:
  // This would fail with the old implementation
  obj.WithAttribute("userId", int64(123456789012345))  // Error!

  // Developers had to do this workaround:
obj.WithAttribute("userId", float64(myInt64Value)) // Ugly + precision
loss

  Solution

  Enhanced the AttributeTypeInt64 case to handle three input types:

  1. int64 - Direct return (no conversion needed)
  2. float64 - Convert with precision validation
  3. string - Parse using strconv.ParseInt

  Key Improvements

  🛡️ Precision Safety: Added validation for float64 → int64 conversions
  - Rejects values outside the safe integer range (±2^53-1)
  - Prevents fractional values from being silently truncated
  - Provides clear error messages when precision would be lost

🚀 Expanded Range: String parsing supports the full int64 range without
precision loss
  - Handles values from -9223372036854775808 to 9223372036854775807
  - No more artificial limitations due to float64 precision boundaries

  ✨ Better Developer Experience:
  - Developers can provide int64 values directly without manual casting
  - Clear, actionable error messages guide developers when issues occur
  - Backward compatible with existing float64 inputs

  Testing

  - ✅ All existing tests continue to pass
  - ✅ Added comprehensive test coverage for new int64 handling scenarios
  - ✅ Added tests for precision validation and error cases
  - ✅ Added integration tests in JSON object processing context

  Breaking Changes

⚠️ Intentional safety improvements that may surface previously silent
issues:
- Values with fractional parts now error instead of truncating silently
- Large integers beyond safe precision now error instead of corrupting
data
  - These are good breaks that improve data integrity
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.

3 participants