-
Notifications
You must be signed in to change notification settings - Fork 12
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
Protect against silent error in root IDs with select_columns
and timestamp
#167
base: master
Are you sure you want to change the base?
Conversation
control logic between |
I don't think this is a good solution philosophically. If we are going to do a workaround at the client level, I think we should auto-insert |
i think the concern is even once we fix server side as you say, someone still could be running an older version of the engine server side and would still have this issue. happy to brainstorm a better solution. im not against the auto insert/strip approach client-side, personally, since that is essentially what this fix asks you to do on your own. thoughts @fcollman? |
As an aside, I think we need to standardize formatting again — this PR has a lot of formatting changes in addition to the code changes. We had previously landed on black before, but it seems like ruff is behaving somewhat differently. Certainly on import order, but also some multiline things. |
actually for import order you might be right, i dont think black touches that if i recall correctly |
@bdpedigo do we want to revist this PR? |
sure, i got a little lost in what the preferred (if temporary) fix would ideally be client side. we talked about
thoughts? |
Related to CAVEconnectome/MaterializationEngine#134
This just raises an error client-side and instructs the user how to temporarily fix.
In the future this will be fixed server-side, but still worth having this here in case someone is on an older version of materialization engine.