Skip to content

(fix): Decimal Parsing Error with e+06 Format#22746

Merged
mergify[bot] merged 2 commits into
matrixorigin:mainfrom
XuPeng-SH:22396
Nov 2, 2025
Merged

(fix): Decimal Parsing Error with e+06 Format#22746
mergify[bot] merged 2 commits into
matrixorigin:mainfrom
XuPeng-SH:22396

Conversation

@XuPeng-SH
Copy link
Copy Markdown
Contributor

@XuPeng-SH XuPeng-SH commented Nov 2, 2025

User description

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

Which issue(s) this PR fixes:

issue #22396

What this PR does / why we need it:

Root Cause

In pkg/container/types/decimal.go, the Parse64 function only handled the minus sign (-) in scientific notation exponents but not the plus sign (+). The Parse128 function already handled both cases correctly.


PR Type

Bug fix, Tests


Description

  • Fix Parse64 function to handle plus sign in scientific notation exponents

  • Add comprehensive unit tests for scientific notation parsing

  • Add distributed test cases for decimal scientific notation support


Diagram Walkthrough

flowchart LR
  A["Parse64 Function"] -->|Add plus sign handling| B["Scientific Notation Support"]
  B -->|e+06, e-06 formats| C["Decimal Parsing"]
  D["Unit Tests"] -->|TestParse64ScientificNotation| E["Test Coverage"]
  F["Distributed Tests"] -->|decimal_scientific_notation| E
  C --> G["Issue #22396 Fixed"]
Loading

File Walkthrough

Relevant files
Bug fix
decimal.go
Add plus sign support to scientific notation parsing         

pkg/container/types/decimal.go

  • Added handling for plus sign (+) in scientific notation exponents
  • Mirrors existing logic for minus sign (-) already present in the
    function
  • Allows Parse64 to correctly parse formats like 1.23456789e+06
+4/-0     
Tests
decimal_test.go
Add scientific notation unit tests for Parse64                     

pkg/container/types/decimal_test.go

  • Added new test function TestParse64ScientificNotation with four test
    cases
  • Tests e+06 format (the bug case from issue [Bug]: decimal parsing error #22396)
  • Tests e6 format without explicit plus sign
  • Tests e-06 format with negative exponent and e+2 format with small
    exponent
  • Validates correct parsing and expected decimal values
+43/-0   
decimal_scientific_notation.test
Add distributed test cases for scientific notation             

test/distributed/cases/dtype/decimal_scientific_notation.test

  • Created new distributed test file for decimal scientific notation
  • Tests seven scenarios including e+06, e6, e-06, and e+2 formats
  • Validates both positive and negative decimal values
  • Verifies consistency between different notation formats
+45/-0   
decimal_scientific_notation.result
Add expected results for distributed tests                             

test/distributed/cases/dtype/decimal_scientific_notation.result

  • Expected output results for distributed test cases
  • Validates correct decimal parsing and formatting
  • Shows expected values for all seven test scenarios
+42/-0   

@qodo-code-review
Copy link
Copy Markdown

qodo-code-review Bot commented Nov 2, 2025

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status: Passed

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status:
No auditing: The new parsing branch handling '+' in scientific notation does not add or
adjust any logging or audit events, and there is no evidence in this diff that critical
actions are being logged.

Referred Code
if x[i] == '+' && floatflag {
	i++
	continue
}
Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status:
Ambiguous names: Variable names like 'floatflag' and 'scalesign' appear terse and could
be unclear without broader context not visible in this diff.

Referred Code
if x[i] == '-' && floatflag {
	scalesign = true
	i++
	continue
}
if x[i] == '+' && floatflag {
	i++
	continue
Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status:
Error content: The error messages include the full input string in user-visible errors, which may be
acceptable but could risk exposing sensitive input depending on usage context outside this
diff.

Referred Code
	err = moerr.NewInvalidInputNoCtxf("%s is illegal string, can't be converted to Decimal64.", x)
	return
} else if !floatflag {
	if width == 18 {
		if scale == -1 {
			err = moerr.NewInvalidInputNoCtxf("%s beyond the range, can't be converted to Decimal64.", x)
Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status:
Input echoing: The error path echoes back the raw input 'x' in formatted error strings, which
could reveal user-supplied data; appropriateness depends on how these errors are surfaced
externally.

Referred Code
	err = moerr.NewInvalidInputNoCtxf("%s is illegal string, can't be converted to Decimal64.", x)
	return
} else if !floatflag {
	if width == 18 {
		if scale == -1 {
			err = moerr.NewInvalidInputNoCtxf("%s beyond the range, can't be converted to Decimal64.", x)
  • Update
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@mergify mergify Bot added the kind/bug Something isn't working label Nov 2, 2025
@qodo-code-review
Copy link
Copy Markdown

qodo-code-review Bot commented Nov 2, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Prevent multiple exponent signs

Improve scientific notation parsing by ensuring an exponent sign (+ or -) can
only appear immediately after e or E, preventing acceptance of multiple signs.

pkg/container/types/decimal.go [1558-1566]

 			if x[i] == '-' && floatflag {
-				scalesign = true
-				i++
-				continue
+				if i > 0 && (x[i-1] == 'e' || x[i-1] == 'E') {
+					scalesign = true
+					i++
+					continue
+				}
 			}
 			if x[i] == '+' && floatflag {
-				i++
-				continue
+				if i > 0 && (x[i-1] == 'e' || x[i-1] == 'E') {
+					i++
+					continue
+				}
 			}

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that the parser accepts malformed scientific notation like 1e+-5 and proposes a valid fix to reject such inputs, improving the parser's robustness.

Medium
  • Update

@mergify mergify Bot added the queued label Nov 2, 2025
@mergify mergify Bot merged commit c019181 into matrixorigin:main Nov 2, 2025
19 checks passed
@mergify mergify Bot removed the queued label Nov 2, 2025
XuPeng-SH added a commit to XuPeng-SH/matrixone that referenced this pull request Nov 5, 2025
### Root Cause
In `pkg/container/types/decimal.go`, the `Parse64` function only handled the minus sign (`-`) in scientific notation exponents but not the plus sign (`+`). The `Parse128` function already handled both cases correctly.

Approved by: @heni02
fengttt pushed a commit to fengttt/matrixone that referenced this pull request May 7, 2026
### Root Cause
In `pkg/container/types/decimal.go`, the `Parse64` function only handled the minus sign (`-`) in scientific notation exponents but not the plus sign (`+`). The `Parse128` function already handled both cases correctly.

Approved by: @heni02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/bug Something isn't working Review effort 2/5 size/M Denotes a PR that changes [100,499] lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants