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

Remove types from TimeAndDims, they aren't needed. #2865

Merged
merged 1 commit into from
May 3, 2016

Conversation

gianm
Copy link
Contributor

@gianm gianm commented Apr 21, 2016

No description provided.

@gianm gianm added this to the 0.9.1 milestone Apr 21, 2016
@navis
Copy link
Contributor

navis commented Apr 21, 2016

I think @jon-wei has a plan for that.

@gianm
Copy link
Contributor Author

gianm commented Apr 21, 2016

I was thinking the types are still known and available through dimensionDescs, so it should not be necessary to put them in the TimeAndDims too.

@himanshug
Copy link
Contributor

it depends on what is the eventual plan , can same dimension column have different type while data is ingested (say user was using dimension-less ingestion and hadn't specified any type in the beginning)... now what should be the final type of that dimension column given that we see some events with type float, some with long, some with string etc.

FWIW, i think it is good to not have it in TimeAndDims and have concept of fixing the dimension column type somehow.

@gianm
Copy link
Contributor Author

gianm commented Apr 28, 2016

@jon-wei any thoughts – do you think this patch makes sense given #2760 or not? If it doesn't make sense feel free to close this PR.

@jon-wei
Copy link
Contributor

jon-wei commented Apr 29, 2016

@gianm From our discussion yesterday, I think it makes sense to get rid of types and reference DimensionDesc list instead

@fjy
Copy link
Contributor

fjy commented May 2, 2016

@gianm @jon-wei keep this PR?

@jon-wei
Copy link
Contributor

jon-wei commented May 2, 2016

@fjy let's keep this

@fjy
Copy link
Contributor

fjy commented May 3, 2016

👍

@himanshug himanshug merged commit 40e595c into apache:master May 3, 2016
@gianm gianm deleted the ii-nuke-types branch September 23, 2022 19:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants