Skip to content
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

[PHOENIX-1814] Exponential notation parsing and tests #67

Closed
wants to merge 3 commits into from

Conversation

rangent
Copy link

@rangent rangent commented Apr 14, 2015

No description provided.

@@ -967,9 +967,14 @@ NUMBER
: POSINTEGER
;

// Exponential format is not supported.
DECIMAL
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of creating a DECIMAL (which maps to PDecimal that uses a BigDecimal), create a rule for DOUBLE which matches the e notation and returns a PDouble instead (which uses a Java double). You could potentially conditionally return a PFloat versus a PDouble depending on the precision used, but not sure we really need to do that. This is the way I've seen other databases make it possible to define a literal that's a double instead of a DECIMAL or NUMERIC.

Copy link
Author

Choose a reason for hiding this comment

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

While investigating ParseNodeFactory, it looks like all numbers are parsed as real numbers (big decimal), or whole numbers (slightly more logic to try to fit them into ints, Longs, or BigDecimals). Just to clarify, you want me to create a new method in ParseNodeFactory to try to parse doubles, then update the Antlr tree to use that new method?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think any new methods in ParseNodeFactory will be necessary, as you can use the factory.literal(Object o) method and if the Object is an instanceof Double, it'll do the right thing. You will need the grammar change, but just create a new top level rule for it instead of adding it to DECIMAL. When it matches, you can do something like this:

$ret = factory.literal(Double.valueOf(token.getText()));

Copy link
Author

Choose a reason for hiding this comment

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

How's that look?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it looks good. Not sure if there's an official syntax for parsing an E notation for SQL-92. This requires that the E notation be there, which is what we want. These are all valid then:

.1 e 100
.01 e + 100
1. e -100
1.23 e 200

Copy link
Author

Choose a reason for hiding this comment

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

All of those will work, and we capture 'e' and 'E' as well. The only one that wont work is "+1.5e-7" (listed in the description of https://issues.apache.org/jira/browse/PHOENIX-1814 as an example value). I tested inserting regular numbers (non-exponential notation) with a '+' before them and the value doesn't parse at all.

Copy link
Author

Choose a reason for hiding this comment

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

FWIW this is the notation Python uses to parse numbers in exponential notation.

@@ -985,4 +985,64 @@ public void testFloatingPointMultiplicationUpsert() throws Exception {
assertTrue(rs.next());
assertEquals(-1.0f, rs.getFloat(1), 0.001);
}

@Test
public void testSystemTableHasDoubleForExponentialNumber() throws Exception {
Copy link
Author

Choose a reason for hiding this comment

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

@JamesRTaylor here is a test that verifies that the internal representation of an exponential number is infact a Double.

@stoty stoty mentioned this pull request Feb 10, 2021
@stoty
Copy link
Contributor

stoty commented Aug 1, 2023

This has already been merged.

@stoty stoty closed this Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants