You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "gortiz (via GitHub)" <gi...@apache.org> on 2024/02/06 16:02:46 UTC
[PR] First attempt to support timestamp implicit columns in V2 [pinot]
gortiz opened a new pull request, #12375:
URL: https://github.com/apache/pinot/pull/12375
As explained in [the documentation](https://docs.pinot.apache.org/basics/indexing/timestamp-index#usage), 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.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
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
Re: [PR] First attempt to support timestamp implicit columns in V2 [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on PR #12375:
URL: https://github.com/apache/pinot/pull/12375#issuecomment-1930195504
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.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
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
Re: [PR] First attempt to support timestamp implicit columns in V2 [pinot]
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on PR #12375:
URL: https://github.com/apache/pinot/pull/12375#issuecomment-1930274399
> 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.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
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
Re: [PR] First attempt to support timestamp implicit columns in V2 [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #12375:
URL: https://github.com/apache/pinot/pull/12375#discussion_r1480126633
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/catalog/PinotCatalog.java:
##########
@@ -63,10 +64,12 @@ public PinotCatalog(TableCache tableCache) {
public Table getTable(String name) {
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");
Review Comment:
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
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
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
Re: [PR] First attempt to support timestamp implicit columns in V2 [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #12375:
URL: https://github.com/apache/pinot/pull/12375#issuecomment-1930429005
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/12375?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: `5 lines` in your changes are missing coverage. Please review.
> Comparison is base [(`041e040`)](https://app.codecov.io/gh/apache/pinot/commit/041e04078f5a94fca92c805a8db8fdf1f904a985?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 61.73% compared to head [(`00abdfd`)](https://app.codecov.io/gh/apache/pinot/pull/12375?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 27.69%.
> Report is 12 commits behind head on master.
| [Files](https://app.codecov.io/gh/apache/pinot/pull/12375?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...ava/org/apache/pinot/query/catalog/PinotTable.java](https://app.codecov.io/gh/apache/pinot/pull/12375?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcGxhbm5lci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvY2F0YWxvZy9QaW5vdFRhYmxlLmphdmE=) | 0.00% | [3 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/12375?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...a/org/apache/pinot/query/catalog/PinotCatalog.java](https://app.codecov.io/gh/apache/pinot/pull/12375?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcGxhbm5lci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvY2F0YWxvZy9QaW5vdENhdGFsb2cuamF2YQ==) | 0.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/12375?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #12375 +/- ##
=============================================
- Coverage 61.73% 27.69% -34.04%
+ Complexity 1152 201 -951
=============================================
Files 2424 2426 +2
Lines 132526 132711 +185
Branches 20483 20519 +36
=============================================
- Hits 81810 36753 -45057
- Misses 44720 93173 +48453
+ Partials 5996 2785 -3211
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/12375/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/12375/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/12375/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/12375/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/12375/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/12375/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `27.68% <0.00%> (-34.00%)` | :arrow_down: |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/12375/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `27.68% <0.00%> (-33.93%)` | :arrow_down: |
| [skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/12375/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `27.69% <0.00%> (-34.02%)` | :arrow_down: |
| [skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/12375/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `27.67% <0.00%> (-33.92%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/12375/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `27.69% <0.00%> (-34.04%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/12375/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `27.69% <0.00%> (-34.04%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/12375/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/12375/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `27.69% <0.00%> (-0.01%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/pinot/pull/12375?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
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