>From Dmitry Lychagin <[email protected]>: Dmitry Lychagin has posted comments on this change. ( https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/14164 )
Change subject: [NO ISSUE][COMP] Add datetime format for CREATE INDEX ...................................................................... Patch Set 1: (7 comments) https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/14164/1/asterixdb/asterix-lang-sqlpp/src/main/javacc/SQLPP.jj File asterixdb/asterix-lang-sqlpp/src/main/javacc/SQLPP.jj: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/14164/1/asterixdb/asterix-lang-sqlpp/src/main/javacc/SQLPP.jj@1234 PS1, Line 1234: ( <CAST><LEFTPAREN><IDENTIFIER> { expectToken(DEFAULT); } <NULL> Could you refactor this into a separate production (CastDefaultNull)? ------------ <IDENTIFIER> { expectToken(DEFAULT); } <NULL> ( LOOKAHEAD(2) <IDENTIFIER> { propertyName = token.image.toLowerCase(); } propertyValue = StringLiteral() { if (castConfig == null) { castConfig = new HashMap<String, String>(); } castConfig.put(propertyName, propertyValue); } )* ----------- The we can use it here and in ViewSpecification() https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/14164/1/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/entities/Index.java File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/entities/Index.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/14164/1/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/entities/Index.java@340 PS1, Line 340: private final String datetimeFormat; Let's rename these fields to cast*: castDatetimeFormat, castDateFormat, castTimeFormat, so it's clear that they're only related to the cast operation https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/14164/1/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/entitytupletranslators/IndexTupleTranslator.java File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/entitytupletranslators/IndexTupleTranslator.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/14164/1/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/entitytupletranslators/IndexTupleTranslator.java@419 PS1, Line 419: int formatFieldPos = Any way we can reuse the code in DatasetTupleTranslator.createMetadataEntityFromARecord() that does the same thing? (Introduce a helper method somewhere?) https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/14164/1/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/entitytupletranslators/IndexTupleTranslator.java@854 PS1, Line 854: // write field 'Format' Can we reuse the code from ViewDetails.writeDatasetDetailsRecordType() that does the same thing? (introduce a helper method?) https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/14164/1/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/utils/IndexUtil.java File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/utils/IndexUtil.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/14164/1/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/utils/IndexUtil.java@196 PS1, Line 196: public static void validateCastConfiguration(Map<String, String> castConfig, SourceLocation sourceLoc) ViewUtils.validateViewConfiguration() does the same thing? Can we share the code? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/14164/1/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/base/temporal/DateTimeFormatUtils.java File asterixdb/asterix-om/src/main/java/org/apache/asterix/om/base/temporal/DateTimeFormatUtils.java: https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/14164/1/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/base/temporal/DateTimeFormatUtils.java@1303 PS1, Line 1303: public static String getDatetimeFormat(Map<String, String> viewConfig) { Let's change 'viewConfig' to 'config' here and below methods? https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/14164/1/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/base/temporal/DateTimeFormatUtils.java@1315 PS1, Line 1315: public static String getTemporalFormat(IAType targetType, Triple<String, String, String> temporalFormatByType) { This class might not be the best place for these methods. Can we move them to org/apache/asterix/metadata/utils/TypeUtil ? -- To view, visit https://asterix-gerrit.ics.uci.edu/c/asterixdb/+/14164 To unsubscribe, or for help writing mail filters, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Change-Id: I087bb2bd79ba1ccfb9a6bbc910dfdb854b75dc9b Gerrit-Change-Number: 14164 Gerrit-PatchSet: 1 Gerrit-Owner: Ali Alsuliman <[email protected]> Gerrit-Reviewer: Ali Alsuliman <[email protected]> Gerrit-Reviewer: Anon. E. Moose #1000171 Gerrit-Reviewer: Dmitry Lychagin <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Michael Blow <[email protected]> Gerrit-Reviewer: Murtadha Hubail <[email protected]> Gerrit-Reviewer: Till Westmann <[email protected]> Gerrit-Comment-Date: Wed, 24 Nov 2021 01:45:02 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
