From 0b8e0d08ce22fbce8b6cab9c99472b5db18a192d Mon Sep 17 00:00:00 2001 From: Jonathan Metter Date: Tue, 21 Feb 2017 20:57:18 +0100 Subject: [PATCH] tests to demonstrate issues with aggregate function handling 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). #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: - #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 ae89dea20a2bb17846f83b3d71d112ead671b171) --- test/test_integration_aggregate.rb | 148 +++++++++++++++++++++++++++++ 1 file changed, 148 insertions(+) diff --git a/test/test_integration_aggregate.rb b/test/test_integration_aggregate.rb index 75ca0466..ed722d22 100644 --- a/test/test_integration_aggregate.rb +++ b/test/test_integration_aggregate.rb @@ -47,6 +47,127 @@ def test_create_aggregate_with_block assert_equal 6, value end + def test_create_aggregate_with_the_same_function_twice_in_a_query + @db.create_aggregate( "accumulate", 1 ) do + step do |ctx,a| + ctx[:sum] ||= 0 + ctx[:sum] += a.to_i + end + + finalize { |ctx| ctx.result = ctx[:sum] } + end + + values = @db.get_first_row( "select accumulate(a), accumulate(c) from foo" ) + assert_equal 6, values[0] + assert_equal 33, values[1] + end + + def test_create_aggregate_with_two_different_functions + @db.create_aggregate( "accumulate", 1 ) do + step do |ctx,a| + ctx[:sum] ||= 0 + ctx[:sum] += a.to_i + end + + finalize { |ctx| ctx.result = ctx[:sum] } + end + + @db.create_aggregate( "multiply", 1 ) do + step do |ctx,a| + ctx[:sum] ||= 1 + ctx[:sum] *= a.to_i + end + + finalize { |ctx| ctx.result = ctx[:sum] } + end + + # This is likely to crash, as ruby-sqlite overwrites its only reference + # to the "accumulate" handler with the "multiply" handler, although + # SQLite still holds a pointer which it follows on next select. + #GC.start + + values = @db.get_first_row( "select accumulate(a), multiply(c) from foo" ) + assert_equal 6, values[0] + assert_equal 1320, values[1] + + value = @db.get_first_value( "select accumulate(c) from foo") + assert_equal 33, value + + value = @db.get_first_value( "select multiply(a) from foo") + assert_equal 6, value + end + + def test_create_aggregate_overwrite_function + @db.create_aggregate( "accumulate", 1 ) do + step do |ctx,a| + ctx[:sum] ||= 0 + ctx[:sum] += a.to_i + end + + finalize { |ctx| ctx.result = ctx[:sum] } + end + + value = @db.get_first_value( "select accumulate(c) from foo") + assert_equal 33, value + + GC.start + + @db.create_aggregate( "accumulate", 1 ) do + step do |ctx,a| + ctx[:sum] ||= 1 + ctx[:sum] *= a.to_i + end + + finalize { |ctx| ctx.result = ctx[:sum] } + end + + value = @db.get_first_value( "select accumulate(c) from foo") + assert_equal 1320, value + end + + def test_create_aggregate_overwrite_function_with_different_arity + @db.create_aggregate( "accumulate", -1 ) do + step do |ctx,*args| + ctx[:sum] ||= 0 + args.each { |a| ctx[:sum] += a.to_i } + end + + finalize { |ctx| ctx.result = ctx[:sum] } + end + + @db.create_aggregate( "accumulate", 2 ) do + step do |ctx,a,b| + ctx[:sum] ||= 1 + ctx[:sum] *= (a.to_i + b.to_i) + end + + finalize { |ctx| ctx.result = ctx[:sum] } + end + + GC.start + + values = @db.get_first_row( "select accumulate(c), accumulate(a,c) from foo") + assert_equal 33, values[0] + assert_equal 39, values[1] + end + + class CustomException < Exception + end + + def test_create_aggregate_with_exception + @db.create_aggregate( "raiseexception", 1 ) do + step do |ctx,a| + raise CustomException.new( "bogus aggregate handler" ) + end + + finalize { |ctx| ctx.result = 42 } + end + + assert_raise CustomException do + @db.get_first_value( "select raiseexception(a) from foo") + end + end + def test_create_aggregate_with_no_data @db.create_aggregate( "accumulate", 1 ) do step do |ctx,a| @@ -90,6 +211,33 @@ def test_aggregate_initialized_twice assert_equal 2, initialized end + def test_create_aggregate_handler_call_with_wrong_arity + @db.create_aggregate_handler AggregateHandler + + assert_raise (SQLite3::SQLException) do + @db.get_first_value( "select multiply(a,c) from foo" ) + end + end + + class RaiseExceptionAggregateHandler + class << self + def arity; 1; end + def text_rep; SQLite3::Constants::TextRep::ANY; end + def name; "raiseexception"; end + end + def step(ctx, a) + raise CustomException.new( "bogus aggregate handler" ) + end + def finalize(ctx); ctx.result = nil; end + end + + def test_create_aggregate_handler_with_exception + @db.create_aggregate_handler RaiseExceptionAggregateHandler + assert_raise CustomException do + @db.get_first_value( "select raiseexception(a) from foo") + end + end + def test_create_aggregate_handler @db.create_aggregate_handler AggregateHandler value = @db.get_first_value( "select multiply(a) from foo" )