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

Timeseries: Avoid collisions with columns named 'timestamp' #828

Merged
merged 2 commits into from
Dec 26, 2017

Conversation

Algunenano
Copy link
Contributor

PR to avoid name clashes when the dataset column in named timestamp which trigger and invalid GROUP BY so you could get several bins with the same number. For example:

{"aggregation":"decade","offset":0,"timestamp_start":631152000,"bin_width":315576000,"bins_count":3,"bins_start":631152000,"nulls":0,"bins":[{"bin":0,"min":631152000,"max":631152000,"avg":631152000,"freq":123,"timestamp":631152000},{"bin":0,"min":631152000,"max":631152000,"avg":631152000,"freq":119,"timestamp":631152000},{"bin":0,"min":631152000,"max":631152000,"avg":631152000,"freq":122,"timestamp":631152000},{"bin":0,"min":631152000,"max":631152000,"avg":631152000,"freq":120,"timestamp":631152000},{"bin":1,"min":946684800,"max":946684800,"avg":946684800,"freq":116,"timestamp":946684800},{"bin":1,"min":946684800,"max":946684800,"avg":946684800,"freq":127,"timestamp":946684800},{"bin":1,"min":946684800,"max":946684800,"avg":946684800,"freq":120,"timestamp":946684800},{"bin":1,"min":946684800,"max":946684800,"avg":946684800,"freq":129,"timestamp":946684800},{"bin":1,"min":946684800,"max":946684800,"avg":946684800,"freq":126,"timestamp":946684800},{"bin":1,"min":946684800,"max":946684800,"avg":946684800,"freq":124,"timestamp":946684800},{"bin":1,"min":946684800,"max":946684800,"avg":946684800,"freq":125,"timestamp":946684800},{"bin":2,"min":1262304000,"max":1262304000,"avg":1262304000,"freq":116,"timestamp":1262304000},{"bin":2,"min":1262304000,"max":1262304000,"avg":1262304000,"freq":110,"timestamp":1262304000},{"bin":2,"min":1262304000,"max":1262304000,"avg":1262304000,"freq":112,"timestamp":1262304000},{"bin":2,"min":1262304000,"max":1262304000,"avg":1262304000,"freq":117,"timestamp":1262304000},{"bin":2,"min":1262304000,"max":1262304000,"avg":1262304000,"freq":113,"timestamp":1262304000}],"type":"histogram"}

Now:

{"aggregation":"decade","offset":0,"timestamp_start":631152000,"bin_width":315576000,"bins_count":3,"bins_start":631152000,"nulls":0,"bins":[{"bin":0,"min":631152000,"max":631152000,"avg":631152000,"freq":607,"timestamp":631152000},{"bin":1,"min":946684800,"max":946684800,"avg":946684800,"freq":1241,"timestamp":946684800},{"bin":2,"min":1262304000,"max":1262304000,"avg":1262304000,"freq":568,"timestamp":1262304000}],"type":"histogram"}

Fixes #827

@Algunenano
Copy link
Contributor Author

In the numeric histograms we there could be issues with columns named 'bins' so I've applied the same principle to avoid it.

Should fix https://github.com/CartoDB/support/issues/1181

Copy link
Contributor

@simon-contreras-deel simon-contreras-deel left a comment

Choose a reason for hiding this comment

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

I don't like to group by an ordinal number of an output column because it is harder to understand and maintain.

But as you said in #827 (comment) changing the name of the output column could have the same problem in the future.

--> approve

@simon-contreras-deel simon-contreras-deel merged commit 524d5a5 into CartoDB:master Dec 26, 2017
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.

2 participants