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

Fix #1080: vtable uses an explicit Omit flag now #1081

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

cheezypoofs
Copy link

@cheezypoofs cheezypoofs commented Aug 31, 2022

The virtual table omit flag is idenfified by sqlite as an optional
hint you can give the database about whether you will process the
constraints, but the library doesn't require that you actually do
the filtering. Separting the .Used flag, which tells this go
wrapper to setup arguments to receive the constraints in Filter,
from the Omit flag allows us to inspect and perhaps do a best-effort
filtering and still allow sqlite proper to complete the evaluation
by leaving .omit false.

The virtual table omit flag is idenfified by sqlite as an optional
hint you can give the database about whether you are processed the
constraints, but the library doesn't require that you actually do
the filtering. Separting the .Used flag, which tells this go
wrapper to setup arguments to receive the constraints in Filter,
from the Omit flag allows us to inspect and perhaps do a best-effort
filtering and still allow sqlite proper to complete the evaluation
by leaving .omit false.
@mattn
Copy link
Owner

mattn commented Sep 18, 2022

AFAICS, this seems to be breaking compatibility. @rittneje what do you think?

@rittneje
Copy link
Collaborator

rittneje commented Sep 18, 2022

@mattn Yes, it is a breaking change. Previously omit was always set to 1 (i.e., true). Now it it will (presumably) default to 0 (i.e., false) unless you set the new Omit field.

I think to properly preserve backwards compatibility, the condition needs to be if res.Omit == nil || res.Omit[i]. That way, all existing code (which will leave Omit as nil) will continue to set it as it does, and new code (which will assign Omit to a non-nil slice) can control it per index. Also, if res.Omit != nil && len(res.Omit) != len(slice) we should probably return an error (like we do for res.Used) as that seems like a developer mistake.

That said, the existing default logic for omit should be explicitly documented as it is the opposite of the default behavior in SQLite.

@cheezypoofs
Copy link
Author

Thanks for the consideration and feedback. I think I have reworked as you suggested.

@rittneje
Copy link
Collaborator

@cheezypoofs Can you add a unit test that leverages the new functionality?

@cheezypoofs
Copy link
Author

Hmmm, I'll have to think of a way to properly exercise it. I'll give it a go.

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.

3 participants