[V1] Use consistent names across PType and Datum #1546
Conversation
Conformance comparison report-Cross Engine
Number failing in both: 77 Number passing in legacy engine but fail in eval engine: 155 Number failing in legacy engine but pass in eval engine: 468 Conformance comparison report-Cross Commit-LEGACY
Number failing in both: 544 Number passing in Base (8717275) but now fail: 1 Number failing in Base (8717275) but now pass: 0 Click here to see
Conformance comparison report-Cross Commit-EVAL
Number failing in both: 232 Number passing in Base (8717275) but now fail: 1 Number failing in Base (8717275) but now pass: 1 Click here to see
Click here to see
|
| @NotNull | ||
| static Datum tinyInt(byte value) { | ||
| return new DatumByte(value, PType.typeTinyInt()); | ||
| static Datum tinyint(byte value) { |
There was a problem hiding this comment.
Any reason you opted to remove the camel-case? Also, does this leave a green-squiggly on your IDE?
There was a problem hiding this comment.
No squiggly, but the naming is quite simple – spaces are underscores in snake, or capital for camel.
However, note that most types don't have spaces.
Examples
TINYINT -- sql
tinyint() -- method
DOUBLE PRECISION -- sql
doublePrecision() -- method
VARCHAR -- sql
varchar() -- method
CHARACTER VARYING -- sql
characterVarying() -- method
Not everything is that consistent because of reserved words in Java, and I've chosen to shorten some redundant/wordy things – for example TIME_WITH_TIMEZONE is easy to shorten to TIMEZ
| } | ||
|
|
||
| @NotNull | ||
| static Datum symbol(@NotNull String value) { | ||
| return new DatumString(value, PType.typeSymbol()); | ||
| static Datum timeWithoutTZ(@NotNull Time value) { |
There was a problem hiding this comment.
From your naming in PType, I would've thought that the static method would be called time and timez.
There was a problem hiding this comment.
It should've been, I need to update.
| @@ -236,12 +236,12 @@ internal object CastTable { | |||
| ) | |||
| } | |||
| register(SMALLINT, DECIMAL_ARBITRARY) { x, _ -> | |||
There was a problem hiding this comment.
With the renaming of INT_ARBITRARY, I would've thought you would remove DECIMAL_ARBITRARY. Is this reserved for another PR?
There was a problem hiding this comment.
Another PR. I started to do some of the things listed in the follow-ups, but then the PR got too complicated.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## v1 #1546 +/- ##
=====================================
Coverage ? 77.18%
Complexity ? 2514
=====================================
Files ? 254
Lines ? 18583
Branches ? 3517
=====================================
Hits ? 14343
Misses ? 3208
Partials ? 1032
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Description
This PR updates the PType and Datum classes to use consistent static method names for types/values.
Follow-Up Considerations
Other Information
and Code Style Guidelines? YES
License Information
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.