Skip to content

Conversation

@rangeroob
Copy link
Owner

Added tests to demonstrate three issues with the way sqlite3-ruby
currently handles aggregate functions:

(1) Defining a second aggregate function and using both results in
memory violation. The problem is that rb_iv_set(self, "@agregator", aggregator); (database.c:399) just overwrites the last reference to
the first AggregateHandler, but SQLite still holds a pointer to it and
will follow the pointer if the first aggregate function is used in a
query.
The most straight-forward fix is to insert the aggregator in a ruby
array and never release it -- similar to how its done for functions.

(2) Using the same aggregate function twice in a query mixes up values
from both columns. For example:
SELECT MYAGGREG(a), MYAGGREG(b) FROM foo;
Leads to: Myaggreg.step(a1); Myaggreg.step(b1); Myaggreg.step(a2);
Myaggreg.step(b2); ... ; Myaggreg.finalize(), Myaggreg.finalize()
The SQLite API expects the caller to differentiate between these
chains of invocation via sqlite3_aggregate_context(), but current
sqlite3-ruby does not account for that.
#44 has been a work around for this in the special case that the first
aggregation is finalized before the second started (separate queries).
sparklemotion#161 does the analog for the function Proxy.

(3) Documentation implies that library users shall explicitly set the
arity of the function. Consequently the library user is likely to
expect that this is passed down to SQLite, so he may define multiple
functions with the same name but different arity and SQLite to use the
most appropriate one (documented sqlite feature). Unfortunately,
sqlite3-ruby does not pass the arity to SQLite, which is surprising
given that different arity-values are accounted for in the C code. The
problem is that sqlite3-ruby does not call the AggregateHandlers
"arity" method but instead queries the arity of
the "step" method from Ruby (sqlite3_obj_method_arity). Confusingly,
this is not the "step" provided by the library user, but the "step" of
the Proxy Wrapper classes introduced by Database.create_aggregate or
Database.create_aggregate_handler. Both of these are just
step(*args), so the arity reported to SQLite is always -1.

Things not addressed:

(cherry picked from commit ae89dea)

Added tests to demonstrate three issues with the way sqlite3-ruby
currently handles aggregate functions:

(1) Defining a second aggregate function and using both results in
memory violation. The problem is that `rb_iv_set(self, "@agregator",
aggregator);`   (database.c:399) just overwrites the last reference to
the first  AggregateHandler, but SQLite still holds a pointer to it and
will follow   the pointer if the first aggregate function is used in a
query.
  The most straight-forward fix is to insert the aggregator in a ruby
array and never release it -- similar to how its done for functions.

(2) Using the same aggregate function twice in a query mixes up values
from both  columns. For example:
  `SELECT MYAGGREG(a), MYAGGREG(b) FROM foo;`
  Leads to: Myaggreg.step(a1); Myaggreg.step(b1); Myaggreg.step(a2);
  Myaggreg.step(b2); ... ; Myaggreg.finalize(), Myaggreg.finalize()
  The SQLite API expects the caller to differentiate between these
chains  of invocation via `sqlite3_aggregate_context()`, but current
sqlite3-ruby  does not account for that.
  #44 has been a work around for this in the special case that the first
  aggregation is finalized before the second started (separate queries).
  sparklemotion#161 does the analog for the function Proxy.

(3) Documentation implies that library users shall explicitly set the
arity of the function. Consequently the library user is likely to
expect that this is passed down to SQLite, so he may define multiple
functions with the same name but different arity and SQLite to use the
most appropriate one  (documented sqlite feature). Unfortunately,
sqlite3-ruby does not pass the arity to SQLite, which is surprising
given that different arity-values  are accounted for in the C code. The
problem is that sqlite3-ruby does not call the AggregateHandlers
"arity" method but instead queries the arity of
  the "step" method from Ruby (sqlite3_obj_method_arity). Confusingly,
this  is not the "step" provided by the library user, but the "step" of
the  Proxy Wrapper classes introduced by Database.create_aggregate or
Database.create_aggregate_handler. Both of these are just
`step(*args)`,  so the arity reported to SQLite is always -1.

Things not addressed:
  - sparklemotion#164 impossible to call FunctionProxy.set_error (and
FunctionProxy.count).  because @driver does not exist (anymore)
  - Database.create_aggregate and the tests contain a 'text_rep' field,
that  is never passed down to SQLite. It is not advertised in the
documentation, though.

(cherry picked from commit ae89dea)
@rangeroob rangeroob merged commit 23c7819 into 1-4 Apr 11, 2020
@rangeroob rangeroob deleted the Merge-ae89dea20a2bb17846f83b3d71d112ead671b171 branch April 11, 2020 21:11
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