You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2020/05/16 01:20:51 UTC

[GitHub] [incubator-pinot] npawar opened a new pull request #5399: DATE_TIME should work as the primary time column for Pinot tables

npawar opened a new pull request #5399:
URL: https://github.com/apache/incubator-pinot/pull/5399


   https://github.com/apache/incubator-pinot/issues/2756
   
   This PR ensures that Pinot can use a DateTimeFieldSpec as a primary time column for the table.
   
   After this change, we no longer need to use TimeFieldSpec. TimeFieldSpec, TimeGranularitySpec have been marked as Deprecated.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] haibow commented on a change in pull request #5399: DATE_TIME should work as the primary time column for Pinot tables

Posted by GitBox <gi...@apache.org>.
haibow commented on a change in pull request #5399:
URL: https://github.com/apache/incubator-pinot/pull/5399#discussion_r426114511



##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/routing/timeboundary/TimeBoundaryManager.java
##########
@@ -34,6 +34,8 @@
 import org.apache.pinot.common.utils.CommonConstants;
 import org.apache.pinot.spi.config.table.TableConfig;
 import org.apache.pinot.spi.config.table.TableType;
+import org.apache.pinot.spi.data.DateTimeFieldSpec;
+import org.apache.pinot.spi.data.DateTimeFormatSpec;

Review comment:
       Please format this file. Has unused imports.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] npawar commented on a change in pull request #5399: DATE_TIME should work as the primary time column for Pinot tables

Posted by GitBox <gi...@apache.org>.
npawar commented on a change in pull request #5399:
URL: https://github.com/apache/incubator-pinot/pull/5399#discussion_r426924704



##########
File path: pinot-core/src/main/java/org/apache/pinot/core/segment/creator/impl/SegmentColumnarIndexCreator.java
##########
@@ -362,12 +362,14 @@ private void writeMetadata()
     properties.setProperty(DIMENSIONS, config.getDimensions());
     properties.setProperty(METRICS, config.getMetrics());
     properties.setProperty(DATETIME_COLUMNS, config.getDateTimeColumnNames());
-    properties.setProperty(TIME_COLUMN_NAME, config.getTimeColumnName());
+    String timeColumnName = config.getTimeColumnName();
+    if (timeColumnName != null && !config.getSchema().getDateTimeNames().contains(timeColumnName)) {

Review comment:
       Summarizing our offline discussion:
   The second part was to avoid setting the time column name in `segment.datetime.column.names` and also in `segment.time.column.name`, which will happen if the primary time column is **dateTimeFieldSpec**. This happens because `segment.time.column.name` was **serving as primary timeColumnName and also as timeFieldSpec.** This worked well so far, because primary time column and timeFieldSpec were one and the same. I added that check, with the intention to stop having a record of primary time column in the segment metadata. It was not being used anywhere in Pinot.
   But based on our chat, removing that check. As decided, the `segment.time.column.name` property will serve as the primary time column name. It will match with timecolumnName in tableConfig. It can be of type DATE_TIME or TIME.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] mcvsubbu commented on a change in pull request #5399: DATE_TIME should work as the primary time column for Pinot tables

Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #5399:
URL: https://github.com/apache/incubator-pinot/pull/5399#discussion_r427479912



##########
File path: pinot-core/src/main/java/org/apache/pinot/core/segment/creator/impl/V1Constants.java
##########
@@ -51,6 +51,10 @@
       public static final String TABLE_NAME = "segment.table.name";
       public static final String DIMENSIONS = "segment.dimension.column.names";
       public static final String METRICS = "segment.metric.column.names";
+      /**
+       * The primary time column for the table. This will match the timeColumnName defined in the tableConfig.
+       * In the Pinot schema, this column can be defined as either a TimeFieldSpec or DateTimeFieldSpec

Review comment:
       mention in the comment that TimeFieldSpec is deprecated




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] mcvsubbu commented on pull request #5399: DATE_TIME should work as the primary time column for Pinot tables

Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on pull request #5399:
URL: https://github.com/apache/incubator-pinot/pull/5399#issuecomment-631129692


   Really appreciate your diligence @npawar thanks


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] npawar merged pull request #5399: DATE_TIME should work as the primary time column for Pinot tables

Posted by GitBox <gi...@apache.org>.
npawar merged pull request #5399:
URL: https://github.com/apache/incubator-pinot/pull/5399


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] codecov-io edited a comment on pull request #5399: DATE_TIME should work as the primary time column for Pinot tables

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #5399:
URL: https://github.com/apache/incubator-pinot/pull/5399#issuecomment-629578859


   # [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/5399?src=pr&el=h1) Report
   > Merging [#5399](https://codecov.io/gh/apache/incubator-pinot/pull/5399?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/430d48e3fb97ad66905b9fad117fd7de3ad2952e&el=desc) will **increase** coverage by `0.03%`.
   > The diff coverage is `73.68%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-pinot/pull/5399/graphs/tree.svg?width=650&height=150&src=pr&token=4ibza2ugkz)](https://codecov.io/gh/apache/incubator-pinot/pull/5399?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master    #5399      +/-   ##
   ==========================================
   + Coverage   66.81%   66.84%   +0.03%     
   ==========================================
     Files        1091     1091              
     Lines       55783    55782       -1     
     Branches     8365     8363       -2     
   ==========================================
   + Hits        37273    37290      +17     
   + Misses      15762    15744      -18     
     Partials     2748     2748              
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/5399?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [...he/pinot/controller/util/AutoAddInvertedIndex.java](https://codecov.io/gh/apache/incubator-pinot/pull/5399/diff?src=pr&el=tree#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci91dGlsL0F1dG9BZGRJbnZlcnRlZEluZGV4LmphdmE=) | `0.00% <0.00%> (ø)` | |
   | [...t/core/data/function/FunctionEvaluatorFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/5399/diff?src=pr&el=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL2Z1bmN0aW9uL0Z1bmN0aW9uRXZhbHVhdG9yRmFjdG9yeS5qYXZh) | `97.43% <ø> (ø)` | |
   | [...va/org/apache/pinot/core/minion/SegmentPurger.java](https://codecov.io/gh/apache/incubator-pinot/pull/5399/diff?src=pr&el=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9taW5pb24vU2VnbWVudFB1cmdlci5qYXZh) | `76.92% <0.00%> (ø)` | |
   | [...tion/batch/common/SegmentGenerationTaskRunner.java](https://codecov.io/gh/apache/incubator-pinot/pull/5399/diff?src=pr&el=tree#diff-cGlub3QtcGx1Z2lucy9waW5vdC1iYXRjaC1pbmdlc3Rpb24vcGlub3QtYmF0Y2gtaW5nZXN0aW9uLWNvbW1vbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcGx1Z2luL2luZ2VzdGlvbi9iYXRjaC9jb21tb24vU2VnbWVudEdlbmVyYXRpb25UYXNrUnVubmVyLmphdmE=) | `0.00% <0.00%> (ø)` | |
   | [...rc/main/java/org/apache/pinot/spi/data/Schema.java](https://codecov.io/gh/apache/incubator-pinot/pull/5399/diff?src=pr&el=tree#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZGF0YS9TY2hlbWEuamF2YQ==) | `75.08% <ø> (+2.45%)` | :arrow_up: |
   | [.../java/org/apache/pinot/spi/data/TimeFieldSpec.java](https://codecov.io/gh/apache/incubator-pinot/pull/5399/diff?src=pr&el=tree#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZGF0YS9UaW1lRmllbGRTcGVjLmphdmE=) | `88.37% <ø> (ø)` | |
   | [...java/org/apache/pinot/spi/utils/TimeConverter.java](https://codecov.io/gh/apache/incubator-pinot/pull/5399/diff?src=pr&el=tree#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvVGltZUNvbnZlcnRlci5qYXZh) | `88.88% <ø> (ø)` | |
   | [...oker/routing/timeboundary/TimeBoundaryManager.java](https://codecov.io/gh/apache/incubator-pinot/pull/5399/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy90aW1lYm91bmRhcnkvVGltZUJvdW5kYXJ5TWFuYWdlci5qYXZh) | `89.87% <100.00%> (+0.12%)` | :arrow_up: |
   | [...manager/realtime/HLRealtimeSegmentDataManager.java](https://codecov.io/gh/apache/incubator-pinot/pull/5399/diff?src=pr&el=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvcmVhbHRpbWUvSExSZWFsdGltZVNlZ21lbnREYXRhTWFuYWdlci5qYXZh) | `82.48% <100.00%> (+0.32%)` | :arrow_up: |
   | [...ment/creator/impl/SegmentColumnarIndexCreator.java](https://codecov.io/gh/apache/incubator-pinot/pull/5399/diff?src=pr&el=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zZWdtZW50L2NyZWF0b3IvaW1wbC9TZWdtZW50Q29sdW1uYXJJbmRleENyZWF0b3IuamF2YQ==) | `90.56% <100.00%> (+2.30%)` | :arrow_up: |
   | ... and [36 more](https://codecov.io/gh/apache/incubator-pinot/pull/5399/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/5399?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/5399?src=pr&el=footer). Last update [430d48e...9cc0d14](https://codecov.io/gh/apache/incubator-pinot/pull/5399?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] haibow commented on a change in pull request #5399: DATE_TIME should work as the primary time column for Pinot tables

Posted by GitBox <gi...@apache.org>.
haibow commented on a change in pull request #5399:
URL: https://github.com/apache/incubator-pinot/pull/5399#discussion_r426114553



##########
File path: pinot-core/src/main/java/org/apache/pinot/core/data/function/FunctionEvaluatorFactory.java
##########
@@ -59,7 +59,8 @@ public FunctionEvaluator getExpressionEvaluator(FieldSpec fieldSpec) {
       }
     } else if (fieldSpec.getFieldType().equals(FieldSpec.FieldType.TIME)) {
 
-      // for backward compatible handling of TIME field conversion
+      // Time conversions should be done using DateTimeFieldSpec and treansformFunctions

Review comment:
       nit: s/treansformFunctions/transformFunctions




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] mcvsubbu commented on pull request #5399: DATE_TIME should work as the primary time column for Pinot tables

Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on pull request #5399:
URL: https://github.com/apache/incubator-pinot/pull/5399#issuecomment-630981224


   If you can please do these steps to test it
   * Create a hybrid table with pinot-0.3.0, let it consume some segments, and have some offline segments as well
   * Upgrade to the current version and then do the following tests:
   1. Schema upgrades fine with a new field added (we are still able to get schema, etc.)
   2. queries work fine (show data from old and new segments), even after new offline and realtime segments are pushed
   
   Ideally, do the tests after upgrade controller, and then again after upgrading broker, and then after upgrading server


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] npawar commented on pull request #5399: DATE_TIME should work as the primary time column for Pinot tables

Posted by GitBox <gi...@apache.org>.
npawar commented on pull request #5399:
URL: https://github.com/apache/incubator-pinot/pull/5399#issuecomment-631114033


   > If you can please do these steps to test it
   > 
   > * Create a hybrid table with pinot-0.3.0, let it consume some segments, and have some offline segments as well
   > * Upgrade to the current version and then do the following tests:
   > 
   > 1. Schema upgrades fine with a new field added (we are still able to get schema, etc.)
   > 2. queries work fine (show data from old and new segments), even after new offline and realtime segments are pushed
   > 
   > Ideally, do the tests after upgrade controller, and then again after upgrading broker, and then after upgrading server
   
   Just completed the following test. It all looks green.
   
   Initial setup:
   1. Started ZK and Kafka. Started Controller, Broker, Server using 0.3.0
   2. Created kafka topic and hybrid table
   3. Pushed 2 offline segments. Pushed data to kafka, allowed a segment to complete.
   4. Checked getSchema, checked query, checked data correctness.
   5. Added a field to schema. Reloaded segments. Restarted server.
   6.  Checked getSchema, checked query, checked data correctness.
   
   Controller upgrade:
   1. Upgraded controller to new code
   2. Checked getSchema, checked query, checked data correctness.
   3. Added a field to schema. Reloaded segments. Restarted server.
   4. Checked getSchema, checked query, checked data correctness.
   5. Pushed one offline segment. Pushed some more realtime data letting more segments complete.
   6. Checked getSchema, checked query, checked data correctness.
   
   Broker upgrade:
   1. Upgraded broker to new code
   2. Checked getSchema, checked query, checked data correctness.
   3. Added a field to schema. Reloaded segments. Restarted server.
   4. Checked getSchema, checked query, checked data correctness.
   5. Pushed one offline segment. Pushed some more realtime data letting more segments complete.
   6. Checked getSchema, checked query, checked data correctness.
   
   Server upgrade:
   1. Upgraded server to new code
   2. Checked getSchema, checked query, checked data correctness.
   3. Added a field to schema. Reloaded segments. Restarted server.
   4. Checked getSchema, checked query, checked data correctness.
   5. Pushed one offline segment. Pushed some more realtime data letting more segments complete.
   6. Checked getSchema, checked query, checked data correctness.
   
   
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] codecov-io commented on pull request #5399: DATE_TIME should work as the primary time column for Pinot tables

Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #5399:
URL: https://github.com/apache/incubator-pinot/pull/5399#issuecomment-629578859


   # [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/5399?src=pr&el=h1) Report
   > Merging [#5399](https://codecov.io/gh/apache/incubator-pinot/pull/5399?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/430d48e3fb97ad66905b9fad117fd7de3ad2952e&el=desc) will **increase** coverage by `0.03%`.
   > The diff coverage is `73.68%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-pinot/pull/5399/graphs/tree.svg?width=650&height=150&src=pr&token=4ibza2ugkz)](https://codecov.io/gh/apache/incubator-pinot/pull/5399?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master    #5399      +/-   ##
   ==========================================
   + Coverage   66.81%   66.84%   +0.03%     
   ==========================================
     Files        1091     1091              
     Lines       55783    55782       -1     
     Branches     8365     8363       -2     
   ==========================================
   + Hits        37273    37290      +17     
   + Misses      15762    15744      -18     
     Partials     2748     2748              
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/5399?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [...he/pinot/controller/util/AutoAddInvertedIndex.java](https://codecov.io/gh/apache/incubator-pinot/pull/5399/diff?src=pr&el=tree#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci91dGlsL0F1dG9BZGRJbnZlcnRlZEluZGV4LmphdmE=) | `0.00% <0.00%> (ø)` | |
   | [...t/core/data/function/FunctionEvaluatorFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/5399/diff?src=pr&el=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL2Z1bmN0aW9uL0Z1bmN0aW9uRXZhbHVhdG9yRmFjdG9yeS5qYXZh) | `97.43% <ø> (ø)` | |
   | [...va/org/apache/pinot/core/minion/SegmentPurger.java](https://codecov.io/gh/apache/incubator-pinot/pull/5399/diff?src=pr&el=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9taW5pb24vU2VnbWVudFB1cmdlci5qYXZh) | `76.92% <0.00%> (ø)` | |
   | [...tion/batch/common/SegmentGenerationTaskRunner.java](https://codecov.io/gh/apache/incubator-pinot/pull/5399/diff?src=pr&el=tree#diff-cGlub3QtcGx1Z2lucy9waW5vdC1iYXRjaC1pbmdlc3Rpb24vcGlub3QtYmF0Y2gtaW5nZXN0aW9uLWNvbW1vbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcGx1Z2luL2luZ2VzdGlvbi9iYXRjaC9jb21tb24vU2VnbWVudEdlbmVyYXRpb25UYXNrUnVubmVyLmphdmE=) | `0.00% <0.00%> (ø)` | |
   | [...rc/main/java/org/apache/pinot/spi/data/Schema.java](https://codecov.io/gh/apache/incubator-pinot/pull/5399/diff?src=pr&el=tree#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZGF0YS9TY2hlbWEuamF2YQ==) | `75.08% <ø> (+2.45%)` | :arrow_up: |
   | [.../java/org/apache/pinot/spi/data/TimeFieldSpec.java](https://codecov.io/gh/apache/incubator-pinot/pull/5399/diff?src=pr&el=tree#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZGF0YS9UaW1lRmllbGRTcGVjLmphdmE=) | `88.37% <ø> (ø)` | |
   | [...java/org/apache/pinot/spi/utils/TimeConverter.java](https://codecov.io/gh/apache/incubator-pinot/pull/5399/diff?src=pr&el=tree#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvVGltZUNvbnZlcnRlci5qYXZh) | `88.88% <ø> (ø)` | |
   | [...oker/routing/timeboundary/TimeBoundaryManager.java](https://codecov.io/gh/apache/incubator-pinot/pull/5399/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy90aW1lYm91bmRhcnkvVGltZUJvdW5kYXJ5TWFuYWdlci5qYXZh) | `89.87% <100.00%> (+0.12%)` | :arrow_up: |
   | [...manager/realtime/HLRealtimeSegmentDataManager.java](https://codecov.io/gh/apache/incubator-pinot/pull/5399/diff?src=pr&el=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvcmVhbHRpbWUvSExSZWFsdGltZVNlZ21lbnREYXRhTWFuYWdlci5qYXZh) | `82.48% <100.00%> (+0.32%)` | :arrow_up: |
   | [...ment/creator/impl/SegmentColumnarIndexCreator.java](https://codecov.io/gh/apache/incubator-pinot/pull/5399/diff?src=pr&el=tree#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9zZWdtZW50L2NyZWF0b3IvaW1wbC9TZWdtZW50Q29sdW1uYXJJbmRleENyZWF0b3IuamF2YQ==) | `90.56% <100.00%> (+2.30%)` | :arrow_up: |
   | ... and [36 more](https://codecov.io/gh/apache/incubator-pinot/pull/5399/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/5399?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/5399?src=pr&el=footer). Last update [430d48e...9cc0d14](https://codecov.io/gh/apache/incubator-pinot/pull/5399?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] haibow commented on a change in pull request #5399: DATE_TIME should work as the primary time column for Pinot tables

Posted by GitBox <gi...@apache.org>.
haibow commented on a change in pull request #5399:
URL: https://github.com/apache/incubator-pinot/pull/5399#discussion_r426114279



##########
File path: pinot-broker/src/test/java/org/apache/pinot/broker/routing/timeboundary/TimeBoundaryManagerTest.java
##########
@@ -75,85 +75,112 @@ public void testTimeBoundaryManager() {
     ExternalView externalView = Mockito.mock(ExternalView.class);
 
     for (TimeUnit timeUnit : TimeUnit.values()) {
-      // Test DAILY push table
+      // Test DAILY push table, with timeFieldSpec
       String rawTableName = "testTable_" + timeUnit + "_DAILY";
-      TableConfig tableConfig = getTableConfig(rawTableName, timeUnit, "DAILY");
-      setSchema(rawTableName, timeUnit);
-
-      // Start with no segment
-      TimeBoundaryManager timeBoundaryManager = new TimeBoundaryManager(tableConfig, _propertyStore);
-      Set<String> onlineSegments = new HashSet<>();
-      timeBoundaryManager.init(externalView, onlineSegments);
-      assertNull(timeBoundaryManager.getTimeBoundaryInfo());
-
-      // Add the first segment should update the time boundary
-      String segment0 = "segment0";
-      onlineSegments.add(segment0);
-      setSegmentZKMetadata(rawTableName, segment0, 2, timeUnit);
-      timeBoundaryManager.init(externalView, onlineSegments);
-      verifyTimeBoundaryInfo(timeBoundaryManager.getTimeBoundaryInfo(), timeUnit.convert(1, TimeUnit.DAYS));
-
-      // Add a new segment with larger end time should update the time boundary
-      String segment1 = "segment1";
-      onlineSegments.add(segment1);
-      setSegmentZKMetadata(rawTableName, segment1, 4, timeUnit);
-      timeBoundaryManager.onExternalViewChange(externalView, onlineSegments);
-      verifyTimeBoundaryInfo(timeBoundaryManager.getTimeBoundaryInfo(), timeUnit.convert(3, TimeUnit.DAYS));
-
-      // Add a new segment with smaller end time should not change the time boundary
-      String segment2 = "segment2";
-      onlineSegments.add(segment2);
-      setSegmentZKMetadata(rawTableName, segment2, 3, timeUnit);
-      timeBoundaryManager.onExternalViewChange(externalView, onlineSegments);
-      verifyTimeBoundaryInfo(timeBoundaryManager.getTimeBoundaryInfo(), timeUnit.convert(3, TimeUnit.DAYS));
-
-      // Remove the segment with largest end time should update the time boundary
-      onlineSegments.remove(segment1);
-      timeBoundaryManager.onExternalViewChange(externalView, onlineSegments);
-      verifyTimeBoundaryInfo(timeBoundaryManager.getTimeBoundaryInfo(), timeUnit.convert(2, TimeUnit.DAYS));
-
-      // Change segment ZK metadata without refreshing should not update the time boundary
-      setSegmentZKMetadata(rawTableName, segment2, 5, timeUnit);
-      timeBoundaryManager.onExternalViewChange(externalView, onlineSegments);
-      verifyTimeBoundaryInfo(timeBoundaryManager.getTimeBoundaryInfo(), timeUnit.convert(2, TimeUnit.DAYS));
-
-      // Refresh the changed segment should update the time boundary
-      timeBoundaryManager.refreshSegment(segment2);
-      verifyTimeBoundaryInfo(timeBoundaryManager.getTimeBoundaryInfo(), timeUnit.convert(4, TimeUnit.DAYS));
+      TableConfig tableConfig = getTableConfig(rawTableName, "DAILY");
+      setSchemaTimeFieldSpec(rawTableName, timeUnit);
+      testDailyPushTable(rawTableName, tableConfig, timeUnit, externalView);
 
-      // Test HOURLY push table
+      // Test HOURLY push table, with timeFieldSpec
       rawTableName = "testTable_" + timeUnit + "_HOURLY";
-      tableConfig = getTableConfig(rawTableName, timeUnit, "HOURLY");
-      setSchema(rawTableName, timeUnit);
-      timeBoundaryManager = new TimeBoundaryManager(tableConfig, _propertyStore);
-      onlineSegments = new HashSet<>();
-      onlineSegments.add(segment0);
-      setSegmentZKMetadata(rawTableName, segment0, 2, timeUnit);
-      timeBoundaryManager.init(externalView, onlineSegments);
-      long expectedTimeValue;
-      if (timeUnit == TimeUnit.DAYS) {
-        // Time boundary should be endTime - 1 DAY when time unit is DAYS
-        expectedTimeValue = timeUnit.convert(1, TimeUnit.DAYS);
-      } else {
-        // Time boundary should be endTime - 1 HOUR when time unit is other than DAYS
-        expectedTimeValue = timeUnit.convert(47, TimeUnit.HOURS);
-      }
-      verifyTimeBoundaryInfo(timeBoundaryManager.getTimeBoundaryInfo(), expectedTimeValue);
+      tableConfig = getTableConfig(rawTableName, "HOURLY");
+      setSchemaTimeFieldSpec(rawTableName, timeUnit);
+      testHourlyPushTable(rawTableName, tableConfig, timeUnit, externalView);
+
+      // Test DAILY push table with dateTimeFieldSpec
+      rawTableName = "testTableDateTime_" + timeUnit + "_DAILY";
+      tableConfig = getTableConfig(rawTableName, "DAILY");
+      setSchemaDateTimeFieldSpec(rawTableName, timeUnit);
+      testDailyPushTable(rawTableName, tableConfig, timeUnit, externalView);
+
+      // Test HOURLY push table
+      rawTableName = "testTableDateTime_" + timeUnit + "_HOURLY";
+      tableConfig = getTableConfig(rawTableName, "HOURLY");
+      setSchemaDateTimeFieldSpec(rawTableName, timeUnit);
+      testHourlyPushTable(rawTableName, tableConfig, timeUnit, externalView);
+    }
+  }
+
+  private void testDailyPushTable(String rawTableName, TableConfig tableConfig, TimeUnit timeUnit, ExternalView externalView) {
+    // Start with no segment
+    TimeBoundaryManager timeBoundaryManager = new TimeBoundaryManager(tableConfig, _propertyStore);
+    Set<String> onlineSegments = new HashSet<>();
+    timeBoundaryManager.init(externalView, onlineSegments);
+    assertNull(timeBoundaryManager.getTimeBoundaryInfo());
+
+    // Add the first segment should update the time boundary
+    String segment0 = "segment0";
+    onlineSegments.add(segment0);
+    setSegmentZKMetadata(rawTableName, segment0, 2, timeUnit);
+    timeBoundaryManager.init(externalView, onlineSegments);
+    verifyTimeBoundaryInfo(timeBoundaryManager.getTimeBoundaryInfo(), timeUnit.convert(1, TimeUnit.DAYS));
+
+    // Add a new segment with larger end time should update the time boundary
+    String segment1 = "segment1";
+    onlineSegments.add(segment1);
+    setSegmentZKMetadata(rawTableName, segment1, 4, timeUnit);
+    timeBoundaryManager.onExternalViewChange(externalView, onlineSegments);
+    verifyTimeBoundaryInfo(timeBoundaryManager.getTimeBoundaryInfo(), timeUnit.convert(3, TimeUnit.DAYS));
+
+    // Add a new segment with smaller end time should not change the time boundary
+    String segment2 = "segment2";
+    onlineSegments.add(segment2);
+    setSegmentZKMetadata(rawTableName, segment2, 3, timeUnit);
+    timeBoundaryManager.onExternalViewChange(externalView, onlineSegments);
+    verifyTimeBoundaryInfo(timeBoundaryManager.getTimeBoundaryInfo(), timeUnit.convert(3, TimeUnit.DAYS));
+
+    // Remove the segment with largest end time should update the time boundary
+    onlineSegments.remove(segment1);
+    timeBoundaryManager.onExternalViewChange(externalView, onlineSegments);
+    verifyTimeBoundaryInfo(timeBoundaryManager.getTimeBoundaryInfo(), timeUnit.convert(2, TimeUnit.DAYS));
+
+    // Change segment ZK metadata without refreshing should not update the time boundary
+    setSegmentZKMetadata(rawTableName, segment2, 5, timeUnit);
+    timeBoundaryManager.onExternalViewChange(externalView, onlineSegments);
+    verifyTimeBoundaryInfo(timeBoundaryManager.getTimeBoundaryInfo(), timeUnit.convert(2, TimeUnit.DAYS));
+
+    // Refresh the changed segment should update the time boundary
+    timeBoundaryManager.refreshSegment(segment2);
+    verifyTimeBoundaryInfo(timeBoundaryManager.getTimeBoundaryInfo(), timeUnit.convert(4, TimeUnit.DAYS));
+  }
+
+  private void testHourlyPushTable(String rawTableName, TableConfig tableConfig, TimeUnit timeUnit, ExternalView externalView) {
+    TimeBoundaryManager timeBoundaryManager = new TimeBoundaryManager(tableConfig, _propertyStore);
+    Set<String> onlineSegments = new HashSet<>();
+    String segment0 = "segment0";
+    onlineSegments.add(segment0);
+    setSegmentZKMetadata(rawTableName, segment0, 2, timeUnit);
+    timeBoundaryManager.init(externalView, onlineSegments);
+    long expectedTimeValue;
+    if (timeUnit == TimeUnit.DAYS) {
+      // Time boundary should be endTime - 1 DAY when time unit is DAYS
+      expectedTimeValue = timeUnit.convert(1, TimeUnit.DAYS);
+    } else {
+      // Time boundary should be endTime - 1 HOUR when time unit is other than DAYS
+      expectedTimeValue = timeUnit.convert(47, TimeUnit.HOURS);
     }
+    verifyTimeBoundaryInfo(timeBoundaryManager.getTimeBoundaryInfo(), expectedTimeValue);
   }
 
-  private TableConfig getTableConfig(String rawTableName, TimeUnit timeUnit, String pushFrequency) {
+  private TableConfig getTableConfig(String rawTableName, String pushFrequency) {
     return new TableConfigBuilder(TableType.OFFLINE).setTableName(rawTableName).setTimeColumnName(TIME_COLUMN)
-        .setTimeType(timeUnit.name()).setSegmentPushFrequency(pushFrequency).build();
+        .setSegmentPushFrequency(pushFrequency).build();
   }
 
-  private void setSchema(String rawTableName, TimeUnit timeUnit) {
+  private void setSchemaTimeFieldSpec(String rawTableName, TimeUnit timeUnit) {
     ZKMetadataProvider.setSchema(_propertyStore,
         new Schema.SchemaBuilder().setSchemaName(rawTableName)
             .addTime(new TimeGranularitySpec(FieldSpec.DataType.LONG, timeUnit, TIME_COLUMN), null)
             .build());
   }
 
+  private void setSchemaDateTimeFieldSpec(String rawTableName, TimeUnit timeUnit) {
+    ZKMetadataProvider.setSchema(_propertyStore,
+        new Schema.SchemaBuilder().setSchemaName(rawTableName)
+            .addDateTime(TIME_COLUMN, FieldSpec.DataType.LONG, "1:"+timeUnit+":EPOCH", "1:"+timeUnit)

Review comment:
       nit: please reformat this file




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] mcvsubbu commented on a change in pull request #5399: DATE_TIME should work as the primary time column for Pinot tables

Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #5399:
URL: https://github.com/apache/incubator-pinot/pull/5399#discussion_r426197422



##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/data/TimeFieldSpec.java
##########
@@ -28,6 +28,12 @@
 
 @SuppressWarnings("unused")
 @JsonIgnoreProperties(ignoreUnknown = true)
+@Deprecated
+/**
+ * TimeFieldSpec is deprecated. Use {@link DateTimeFieldSpec} instead.

Review comment:
       ```suggestion
    * @deprecated TimeFieldSpec is deprecated. Use {@link DateTimeFieldSpec} instead.
   ```

##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/data/Schema.java
##########
@@ -507,10 +507,10 @@ public SchemaBuilder addMetric(String metricName, DataType dataType, Object defa
 
     /**
      * Add timeFieldSpec with incoming and outgoing granularity spec
-     * TODO: This is going to be deprecated in favor of addDateTime().
-     *  Many tests use this to construct Schema with TimeFieldSpec.
-     *  This will continue to exist for a while, as it helps to test backward compatibility of schemas containing TimeFieldSpec
+     * Deprecated in favor of addDateTime().

Review comment:
       ```suggestion
        * @deprecated in favor of {@link #addDateTime()}.
   ```




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] Jackie-Jiang commented on a change in pull request #5399: DATE_TIME should work as the primary time column for Pinot tables

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #5399:
URL: https://github.com/apache/incubator-pinot/pull/5399#discussion_r426869167



##########
File path: pinot-core/src/main/java/org/apache/pinot/core/segment/name/NormalizedDateSegmentNameGenerator.java
##########
@@ -60,16 +61,16 @@ public NormalizedDateSegmentNameGenerator(String tableName, @Nullable String seg
       }
       _outputSDF.setTimeZone(TimeZone.getTimeZone("UTC"));
 
-      // Parse input time format: 'EPOCH' or 'SIMPLE_DATE_FORMAT:<pattern>'
-      if (Preconditions.checkNotNull(timeFormat).equals(TimeFormat.EPOCH.toString())) {
-        _inputTimeUnit = timeType;
+      // Parse input time format: 'EPOCH' or 'SIMPLE_DATE_FORMAT' using pattern
+      Preconditions.checkNotNull(dateTimeFormatSpec);
+      TimeFormat timeFormat = dateTimeFormatSpec.getTimeFormat();
+      if (timeFormat.equals(TimeFormat.EPOCH)) {

Review comment:
       ```suggestion
         if (timeFormat == TimeFormat.EPOCH) {
   ```

##########
File path: pinot-core/src/main/java/org/apache/pinot/core/segment/creator/impl/SegmentColumnarIndexCreator.java
##########
@@ -362,12 +362,14 @@ private void writeMetadata()
     properties.setProperty(DIMENSIONS, config.getDimensions());
     properties.setProperty(METRICS, config.getMetrics());
     properties.setProperty(DATETIME_COLUMNS, config.getDateTimeColumnNames());
-    properties.setProperty(TIME_COLUMN_NAME, config.getTimeColumnName());
+    String timeColumnName = config.getTimeColumnName();
+    if (timeColumnName != null && !config.getSchema().getDateTimeNames().contains(timeColumnName)) {

Review comment:
       Why do we need the second part of the check? What if the schema has no DateTimeField but only TimeField?

##########
File path: pinot-core/src/main/java/org/apache/pinot/core/segment/creator/impl/SegmentColumnarIndexCreator.java
##########
@@ -362,12 +362,14 @@ private void writeMetadata()
     properties.setProperty(DIMENSIONS, config.getDimensions());
     properties.setProperty(METRICS, config.getMetrics());
     properties.setProperty(DATETIME_COLUMNS, config.getDateTimeColumnNames());
-    properties.setProperty(TIME_COLUMN_NAME, config.getTimeColumnName());
+    String timeColumnName = config.getTimeColumnName();
+    if (timeColumnName != null && !config.getSchema().getDateTimeNames().contains(timeColumnName)) {
+      properties.setProperty(TIME_COLUMN_NAME, timeColumnName);
+    }
     properties.setProperty(SEGMENT_TOTAL_DOCS, String.valueOf(totalDocs));
 
     // Write time related metadata (start time, end time, time unit)
-    String timeColumn = config.getTimeColumnName();
-    ColumnIndexCreationInfo timeColumnIndexCreationInfo = indexCreationInfoMap.get(timeColumn);
+    ColumnIndexCreationInfo timeColumnIndexCreationInfo = indexCreationInfoMap.get(timeColumnName);

Review comment:
       Check whether `timeColumnName` is `null` first




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] npawar commented on pull request #5399: DATE_TIME should work as the primary time column for Pinot tables

Posted by GitBox <gi...@apache.org>.
npawar commented on pull request #5399:
URL: https://github.com/apache/incubator-pinot/pull/5399#issuecomment-629709153


   > This diff looks like a milestone for TimeFieldSpec deprecation. @npawar shall we include this in 0.4.0 release? We could probably hold off on the release a bit. cc @kishoreg
   
   Yes, this is a milestone for TimeFieldSpec deprecation. Would be great if we can include in the release


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] npawar commented on a change in pull request #5399: DATE_TIME should work as the primary time column for Pinot tables

Posted by GitBox <gi...@apache.org>.
npawar commented on a change in pull request #5399:
URL: https://github.com/apache/incubator-pinot/pull/5399#discussion_r426924704



##########
File path: pinot-core/src/main/java/org/apache/pinot/core/segment/creator/impl/SegmentColumnarIndexCreator.java
##########
@@ -362,12 +362,14 @@ private void writeMetadata()
     properties.setProperty(DIMENSIONS, config.getDimensions());
     properties.setProperty(METRICS, config.getMetrics());
     properties.setProperty(DATETIME_COLUMNS, config.getDateTimeColumnNames());
-    properties.setProperty(TIME_COLUMN_NAME, config.getTimeColumnName());
+    String timeColumnName = config.getTimeColumnName();
+    if (timeColumnName != null && !config.getSchema().getDateTimeNames().contains(timeColumnName)) {

Review comment:
       Summarizing our offline discussion:
   The second part was to avoid setting the time column name in `segment.datetime.column.names` and also in `segment.time.column.name`, which will happen if the primary time column is **dateTimeFieldSpec**. This happens because `segment.time.column.name` is **serving as primary timeColumnName and also as timeFieldSpec.** This worked well so far, because primary time column and timeFieldSpec were one and the same. 
   But based on our chat, removing that check. As decided, the `segment.time.column.name` property will serve as the primary time column name. It will match with timecolumnName in tableConfig. It can be of type DATE_TIME or TIME.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] npawar commented on pull request #5399: DATE_TIME should work as the primary time column for Pinot tables

Posted by GitBox <gi...@apache.org>.
npawar commented on pull request #5399:
URL: https://github.com/apache/incubator-pinot/pull/5399#issuecomment-629714026


   > you have mentioned that you tested schema with no time on hybrid tables. How does that work for query routing?
   
   updated comment


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] haibow commented on a change in pull request #5399: DATE_TIME should work as the primary time column for Pinot tables

Posted by GitBox <gi...@apache.org>.
haibow commented on a change in pull request #5399:
URL: https://github.com/apache/incubator-pinot/pull/5399#discussion_r426114511



##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/routing/timeboundary/TimeBoundaryManager.java
##########
@@ -34,6 +34,8 @@
 import org.apache.pinot.common.utils.CommonConstants;
 import org.apache.pinot.spi.config.table.TableConfig;
 import org.apache.pinot.spi.config.table.TableType;
+import org.apache.pinot.spi.data.DateTimeFieldSpec;
+import org.apache.pinot.spi.data.DateTimeFormatSpec;

Review comment:
       Please reformat all affected files. This file has unused imports.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] haibow commented on pull request #5399: DATE_TIME should work as the primary time column for Pinot tables

Posted by GitBox <gi...@apache.org>.
haibow commented on pull request #5399:
URL: https://github.com/apache/incubator-pinot/pull/5399#issuecomment-629580638


   This diff looks like a milestone for TimeFieldSpec deprecation. @npawar shall we include this in 0.4.0 release? We could probably hold off on the release a bit. cc @kishoreg 


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [incubator-pinot] mcvsubbu commented on pull request #5399: DATE_TIME should work as the primary time column for Pinot tables

Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on pull request #5399:
URL: https://github.com/apache/incubator-pinot/pull/5399#issuecomment-629713879


   you have mentioned that you tested schema with no time on hybrid tables. How does that work for query routing?


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org