Skip to content

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

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.

Sign up for GitHub

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

Closed
rangent wants to merge 3 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fd7084d
Exponential notation parsing and tests
brianesserlieu Apr 14, 2015
fce7bed
Created new DOUBLE literal in lexer
brianesserlieu Apr 14, 2015
f1cd42b
Added test querying SYSTEM.CATALOG
brianesserlieu Apr 14, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 60 additions & 0 deletions phoenix-core/src/it/java/org/apache/phoenix/end2end/ArithmeticQueryIT.java
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -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 {

@rangentrangentApr 14, 2015

Copy link
Copy Markdown
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.

Connection conn = DriverManager.getConnection(getUrl());
String ddl = "CREATE TABLE test (id VARCHAR not null primary key, num FLOAT)";
conn.createStatement().execute(ddl);
String dml = "UPSERT INTO test(id,num) VALUES ('testid', 1.2E3)";
conn.createStatement().execute(dml);
conn.commit();

ResultSet rs = conn.createStatement().executeQuery("SELECT 1.2E3 FROM SYSTEM.CATALOG LIMIT 1");
assertTrue(rs.next());
assertTrue(rs.getObject(1) instanceof Double);
}

@Test
public void testFloatingPointWithExponentialNotation() throws Exception {
Float[] expected = {1.5E7f, 1.5E-7f, -1.5E-7f, 12E-5f, -.12E+34f};
String[] values = {"1.5e7", "1.5e-7", "-1.5e-7", "12E-5", "-.12E+34"};
ResultSet rs = createTableWithValues(values, "FLOAT");
for (int i = 0; i < expected.length; i++) {
assertEquals(expected[i], rs.getFloat(i+1), 0.001);
}
}

@Test
public void testDoubleWithExponentialNotation() throws Exception {
Double[] expected = {1.5E7d, 1.5E-7d, -1.5E-7d, 12E-5d, -.654E-321d, .1234E+56d};
String[] values = {"1.5e7", "1.5e-7", "-1.5e-7", "12E-5", "-.654E-321", ".1234E+56"};
ResultSet rs = createTableWithValues(values, "DOUBLE");
for (int i = 0; i < expected.length; i++) {
assertEquals(expected[i], rs.getDouble(i+1), 0.001);
}
}

@JamesRTaylorJamesRTaylorApr 14, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A good test, once you change PhoenixSQL.g to create a PDouble instead of a PDecimal behind the scenes, would be to do a rs.getObject(1) and assert that the value is an instance of Double.

@rangentrangentApr 14, 2015

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

The ResultSet seems to be forcing the type of the returned value outside of the Parsing/Lexing logic. eg. "rs.getObject(1) instanceof Float" is true and "rs.getObject(1) instanceof Double" is false in testFloatingPointWithExponentialNotation, and the opposite in testDoubleWithExponentialNotation.

@JamesRTaylorJamesRTaylorApr 14, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What does the SQL statement look like that you're running? It's a bit hard to tell from the test itself. I'd stick to something simple like this:

SELECT .1 e 100, .01 e + 100, 1. e -100, 1.23 e 200 FROM SYSTEM.CATALOG LIMIT 1;

And then do the rs.getObject(1), rs.getObject(2), etc.

@rangentrangentApr 14, 2015

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Ultimately it becomes "SELECT num0, num1, num2... num{values.length-1} FROM test"

private ResultSet createTableWithValues(String[] values, String valueType) throws SQLException {
Connection conn = DriverManager.getConnection(getUrl());
StringBuilder ddl = new StringBuilder("CREATE TABLE test (id VARCHAR not null primary key");

@JamesRTaylorJamesRTaylorApr 14, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Since you're really just evaluating SELECT expressions, you could just query against SYSTEM.CATALOG with a LIMIT 1.

@rangentrangentApr 14, 2015

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

James can you clarify? I'm creating a table, inserting a concrete array of values, then selecting those values from that table. I don't think that I can use the metadata around that table to help me with this test?

@JamesRTaylorJamesRTaylorApr 14, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

See comment here. You really just want to test how Phoenix translates the literal to an expression which you can test that way. If you upsert the values into a column, there's quite a bit more going on. We'll do automatic coercion from the source value to the target value, for example.

StringBuilder dmll = new StringBuilder("UPSERT INTO test(id,");
StringBuilder dmlr = new StringBuilder(") VALUES ('testid'");
StringBuilder select = new StringBuilder("SELECT");
for(int i = 0; i < values.length; i++) {
ddl.append(", num").append(i).append(" ").append(valueType);
dmll.append("num").append(i).append(",");
dmlr.append(", ").append(values[i]);
select.append(" num").append(i).append(",");
}
ddl.append(")");
dmlr.append(")");
dmll.deleteCharAt(dmll.length()-1);
select.deleteCharAt(select.length()-1);
select.append(" FROM test");
conn.createStatement().execute(ddl.toString());
conn.createStatement().execute(dmll.toString() + dmlr.toString());
conn.commit();

ResultSet rs = conn.createStatement().executeQuery(select.toString());
rs.next();
return rs;
}
}
16 changes: 14 additions & 2 deletions phoenix-core/src/main/antlr3/PhoenixSQL.g
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -903,6 +903,9 @@ literal returns [LiteralParseNode ret]
| d=DECIMAL {
ret = factory.realNumber(d.getText());
}
| dbl=DOUBLE {
ret = factory.literal(Double.valueOf(dbl.getText()));
}
| NULL {ret = factory.literal(null);}
| TRUE {ret = factory.literal(Boolean.TRUE);}
| FALSE {ret = factory.literal(Boolean.FALSE);}
Expand DownExpand Up@@ -967,9 +970,18 @@ NUMBER
: POSINTEGER
;

// Exponential format is not supported.
DECIMAL

@JamesRTaylorJamesRTaylorApr 14, 2015

Copy link
Copy Markdown
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.

@rangentrangentApr 14, 2015

Copy link
Copy Markdown
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?

@JamesRTaylorJamesRTaylorApr 14, 2015

Copy link
Copy Markdown
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()));

@rangentrangentApr 14, 2015

Copy link
Copy Markdown
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?

@JamesRTaylorJamesRTaylorApr 14, 2015

Copy link
Copy Markdown
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

@rangentrangentApr 14, 2015

Copy link
Copy Markdown
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.

@rangentrangentApr 14, 2015

Copy link
Copy Markdown
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.

: POSINTEGER? '.' POSINTEGER
: POSINTEGER? '.' POSINTEGER
;

DOUBLE
: '.' POSINTEGER Exponent
| POSINTEGER '.' Exponent
| POSINTEGER ('.' (POSINTEGER (Exponent)?)? | Exponent)
;

Exponent
: ('e' | 'E') ( PLUS | MINUS )? POSINTEGER
;

DOUBLE_QUOTE
Expand Down