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

[18.05] row ids that are all numeric become ints... #6639

Merged
merged 2 commits into from Aug 27, 2018

Conversation

bwlang
Copy link
Contributor

@bwlang bwlang commented Aug 26, 2018

Since the "F" prefix is checked with substring, row_id must always be a string

@galaxybot galaxybot added this to the 18.09 milestone Aug 26, 2018
bwlang and others added 2 commits August 26, 2018 20:31
Since the "F" prefix is checked with substring, row_id must always be a string
@mvdbeek mvdbeek changed the base branch from dev to release_18.05 August 26, 2018 18:37
@mvdbeek mvdbeek changed the title row ids that are all numeric become ints... [18.05] row ids that are all numeric become ints... Aug 26, 2018
@mvdbeek mvdbeek requested a review from martenson August 26, 2018 18:38
@martenson
Copy link
Member

@bwlang good catch, I have never seen a numeric id myself yet. Thank you for the fix.

As of the backport to 18.05 - Given 18.09 should be out within a month, and that some static-building dependencies have changed their behaviour (which is causing this PR to have 235 files changed) I would like to avoid this sizable PR and just fix it in dev instead.

Would that be fine @bwlang @mvdbeek ?

@mvdbeek
Copy link
Member

mvdbeek commented Aug 27, 2018

I don't know, if it's broken we should fix it IMHO, unfortunately most deployers like to play it "safe" by staying behind.

@bwlang
Copy link
Contributor Author

bwlang commented Aug 27, 2018

I'll leave the wait vs now decision up to you - i doubt many people get burned by it and I've worked around it locally (thanks to @mvdbeek's static generation for - still not sure why that doesn't work locally for me). Hmm -what's the probability of generating an all numeric id of n characters - might be a fun question to work out ;) for one digit it's 10/36 right? so is that (10/36)^n ??

@martenson
Copy link
Member

@mvdbeek Feel free to merge.

@mvdbeek
Copy link
Member

mvdbeek commented Aug 27, 2018

We've had someone at the GCC who also saw all numeric IDs and it was causing problems when we had to guess if the ID was encoded or not, so it's not that unlikely.
Also they are hex values, so chance of numeric is (10/16)^n, and who knows what biases we have in our hashing scheme.

@mvdbeek mvdbeek merged commit 03c485e into galaxyproject:release_18.05 Aug 27, 2018
@martenson martenson added this to Closed in Data Libraries Aug 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants