go-sqlite3 icon indicating copy to clipboard operation
go-sqlite3 copied to clipboard

Fix #1080: vtable uses an explicit Omit flag now

Open cheezypoofs opened this issue 2 years ago • 5 comments

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.

cheezypoofs avatar Aug 31 '22 20:08 cheezypoofs

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

mattn avatar Sep 18 '22 14:09 mattn

@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.

rittneje avatar Sep 18 '22 20:09 rittneje

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

cheezypoofs avatar Sep 29 '22 10:09 cheezypoofs

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

rittneje avatar Oct 18 '22 22:10 rittneje

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

cheezypoofs avatar Nov 04 '22 08:11 cheezypoofs