Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The last of the getTimeFieldSpec calls #5378

Merged
merged 4 commits into from
May 15, 2020

Conversation

npawar
Copy link
Contributor

@npawar npawar commented May 12, 2020

#2756
Removing the last of the getTimeFieldSpec calls.

  1. SegmentGeneratorConfig now uses the new getSpecForTimeColumn() call, which returns a DateTimeFieldSpec. Time column is expected in table config if it is to be considered when creating segment. (this also sets the stage for allowing a DateTimeFieldSpec to be a primary time column)
  2. Removing TimeFieldSpec special handling from RealtimeSegmentConverter.getUpdatedSchema. There is no need to remove the incoming time spec. Neither the recordReader nor the dataSource require the updated schema. Plus, the record transformer is a pass through.
  3. Removing special casing for TimeFieldSpec in Schema.isBackwardCompatible() method. The for loop for all specs includes time

@npawar npawar force-pushed the remove_timefieldspec_usages branch from d9f15ed to 540db6c Compare May 15, 2020 16:48
@npawar npawar requested review from haibow, mcvsubbu and Jackie-Jiang and removed request for haibow May 15, 2020 16:49
Copy link
Contributor

@Jackie-Jiang Jackie-Jiang left a comment

Choose a reason for hiding this comment

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

Not related to this PR, but inside the Schema can we do the TimeFieldSpec to DateTimeFieldSpec conversion on the setter side instead of the getter side?
The problem of conversion on the getter side is that the schema still stores the deprecated TimeFieldSpec, and getFieldSpecMap() and getFieldSpecFor() will still return the TimeFieldSpec instead of DateTimeFieldSpec. This can easily cause bugs inside the code.


@Test
public void testNoTimeColumnsInSchema() {
Schema schema = new Schema();
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still want this support? (Realtime table without time column)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

realtime table without time will never happen right?
Also after changing the getUpdatedSchema method, this test becomes irrelevant

Copy link
Contributor

Choose a reason for hiding this comment

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

I think @fx19880617 added that support. Any opinion on this?

@npawar
Copy link
Contributor Author

npawar commented May 15, 2020

Not related to this PR, but inside the Schema can we do the TimeFieldSpec to DateTimeFieldSpec conversion on the setter side instead of the getter side?
The problem of conversion on the getter side is that the schema still stores the deprecated TimeFieldSpec, and getFieldSpecMap() and getFieldSpecFor() will still return the TimeFieldSpec instead of DateTimeFieldSpec. This can easily cause bugs inside the code.

That was exactly the initial plan - to modify the setter to save it as a DateTimeFieldspec. But now we changed the plan. We are not going to do any changes to ser/deser. Reason mainly being external integrations (presto, superset etc) which still need TimeFieldSpec, and also upgrades. I've updated the design doc: https://docs.google.com/document/d/1SU1jCjfsIDSA960fD5YWQbD72p8UdGF0c7CroFNt9Ho/edit#heading=h.6gaudq1t6ys8 (Step C and also put the previous approach in Discarded section)

New plan is to make sure Pinot can treat TIME or DATE_TIME either of them as the primary time column.

@npawar npawar merged commit 430d48e into apache:master May 15, 2020
@npawar npawar deleted the remove_timefieldspec_usages branch May 15, 2020 22:53
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.

None yet

2 participants