Skip to content

First attempt to support timestamp implicit columns in V2#12375

Draft
gortiz wants to merge 1 commit intoapache:masterfrom
gortiz:timestamp-columns-in-v2
Draft

First attempt to support timestamp implicit columns in V2#12375
gortiz wants to merge 1 commit intoapache:masterfrom
gortiz:timestamp-columns-in-v2

Conversation

@gortiz
Copy link
Contributor

@gortiz gortiz commented Feb 6, 2024

As explained in the documentation, timestamp indexes create one new column per defined granularity. These columns are accessible as:

select ts, $ts$DAY from airlineStats limit 10

But the field is not added to Calcite Catalog.

String tableName = TableNameBuilder.extractRawTableName(name);
org.apache.pinot.spi.data.Schema schema = _tableCache.getSchema(tableName);
// TODO: This is not correct!
TableConfig tableConfig = _tableCache.getTableConfig(tableName + "_OFFLINE");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem here is that TableCache is storing TableConfigs indexed by tableNameWithType, but we don't have the type here because Calcite calls this function and it doesn't know about offline and realtime tables.

Here I'm assuming the table will always have an offline part (which AFAIK is not something we can assume) and that in case it has both REALTIME and OFFLINE parts, both are going to be associated with the same table config (which I don't know if it is something we can assume).

The issue we want to solve here only makes sense in OFFLINE tables, but I think we may need to have the TableConfig here in future, so we should either find a solution where the table config is not necessary or find a solution where the table config will always be obtainable.

cc: @walterddr @Jackie-Jiang @xiangfu0

@gortiz gortiz marked this pull request as draft February 6, 2024 16:13
@gortiz
Copy link
Contributor Author

gortiz commented Feb 6, 2024

Consider this a draft. We may need to think whether in V2 we want to keep this behavior or not.

IMHO the fact that a timestamp index creates these virtual fields should not be known by the customer. In case they want to use them they should use minutes(ts) or days(ts) in their SQL expressions. In case there is an index with that granularity, we can change the physical plan to use it. In case there is not, we just apply the operation.

@walterddr
Copy link
Contributor

In case they want to use them they should use minutes(ts) or days(ts) in their SQL expressions. In case there is an index with that granularity, we can change the physical plan to use it. In case there is not, we just apply the operation.

+1 to this approach.

@codecov-commenter
Copy link

codecov-commenter commented Feb 6, 2024

Codecov Report

Attention: Patch coverage is 0% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 27.73%. Comparing base (59551e4) to head (8e91c72).
Report is 523 commits behind head on master.

Files Patch % Lines
...ava/org/apache/pinot/query/catalog/PinotTable.java 0.00% 3 Missing ⚠️
...a/org/apache/pinot/query/catalog/PinotCatalog.java 0.00% 2 Missing ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##             master   #12375       +/-   ##
=============================================
- Coverage     61.75%   27.73%   -34.02%     
+ Complexity      207      192       -15     
=============================================
  Files          2436     2536      +100     
  Lines        133233   139405     +6172     
  Branches      20636    21548      +912     
=============================================
- Hits          82274    38669    -43605     
- Misses        44911    97787    +52876     
+ Partials       6048     2949     -3099     
Flag Coverage Δ
custom-integration1 ?
integration ?
integration1 ?
integration2 ?
java-11 27.73% <0.00%> (-33.98%) ⬇️
java-21 27.73% <0.00%> (-33.89%) ⬇️
skip-bytebuffers-false 27.73% <0.00%> (-34.01%) ⬇️
skip-bytebuffers-true 27.73% <0.00%> (+<0.01%) ⬆️
temurin 27.73% <0.00%> (-34.02%) ⬇️
unittests 27.73% <0.00%> (-34.01%) ⬇️
unittests1 ?
unittests2 27.73% <0.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@gortiz gortiz force-pushed the timestamp-columns-in-v2 branch from 00abdfd to 8e91c72 Compare May 31, 2024 07:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants