From 17dd0bb70d239c1409dfa2f6ab21738f497f1403 Mon Sep 17 00:00:00 2001 From: Georgy Moiseev Date: Fri, 15 Mar 2024 18:32:04 +0300 Subject: [PATCH] scan: fix nil filters Before this patch, some nil conditions failed with internal filter library build error. This patch fixes this internal error, as well as normalize filters to behave similar to core indexes. The logic in core Tarantool select is as follows. `nil` condition in index select is an absence of condition, thus all data is returned disregarding the condition (condition may affect the order). `box.NULL` condition is a condition for the null value -- in case of EQ, only records with null index value are returned, in case of GT, all non-null values are returned since nulls are in the beginning of an index and so on. `nil`s and `box.NULL`s in tuple are both satisfy `box.NULL` equity. After this patch, `nil` filter condition is treated as no condition. This is a breaking change since conditions for `'>'` and `'<'` operator with `nil` operand had resulted with empty response before this patch. But since it was inconsistent with scanning index conditions and wasn't intentional, we change it here. Closes #422 --- CHANGELOG.md | 2 + crud/compare/filters.lua | 36 ++- test/helper.lua | 4 + test/integration/count_test.lua | 8 + test/integration/pairs_readview_test.lua | 8 + test/integration/pairs_test.lua | 8 + test/integration/read_scenario.lua | 327 ++++++++++++++++++++++ test/integration/select_readview_test.lua | 8 + test/integration/select_test.lua | 8 + 9 files changed, 396 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 20928876..243646f3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. non-iterating indexes (#373). * Passing errors from storages for merger operations (`crud.select`, `crud.pairs`, `readview:select`, `readview:pairs`) (#423). +* Working with `nil` operand conditions in case of non-indexed fields or + non-iterating indexes (#422). ## [1.4.3] - 05-02-24 diff --git a/crud/compare/filters.lua b/crud/compare/filters.lua index 985a053e..95061a0d 100644 --- a/crud/compare/filters.lua +++ b/crud/compare/filters.lua @@ -345,9 +345,14 @@ local function gen_eq_func_code(func_name, cond, func_args_code) local header = LIB_FUNC_HEADER_TEMPLATE:format(func_name, func_args_code) table.insert(func_code_lines, header) - local return_line = string.format( - ' return %s', concat_conditions(eq_conds, 'and') - ) + local return_line + if #eq_conds > 0 then + return_line = string.format( + ' return %s', concat_conditions(eq_conds, 'and') + ) + else -- nil condition is treated as no condition + return_line = ' return true' + end table.insert(func_code_lines, return_line) table.insert(func_code_lines, 'end') @@ -398,18 +403,23 @@ local function gen_cmp_array_func_code(operator, func_name, cond, func_args_code local results = results_by_operators[operator] assert(results ~= nil) - for i = 1, #eq_conds do - local comp_value_code = table.concat({ - string.format(' if %s then return %s end', lt_conds[i], results.le), - string.format(' if not %s then return %s end', eq_conds[i], results.not_eq), - '', - }, '\n') + if #eq_conds > 0 then + for i = 1, #eq_conds do + local comp_value_code = table.concat({ + string.format(' if %s then return %s end', lt_conds[i], results.le), + string.format(' if not %s then return %s end', eq_conds[i], results.not_eq), + '', + }, '\n') - table.insert(func_code_lines, comp_value_code) - end + table.insert(func_code_lines, comp_value_code) + end - local return_code = (' return %s'):format(results.default) - table.insert(func_code_lines, return_code) + local return_code = (' return %s'):format(results.default) + table.insert(func_code_lines, return_code) + else -- nil condition is treated as no condition + local return_line = ' return true' + table.insert(func_code_lines, return_line) + end table.insert(func_code_lines, 'end') diff --git a/test/helper.lua b/test/helper.lua index 24efb0e4..0e10eee5 100644 --- a/test/helper.lua +++ b/test/helper.lua @@ -962,6 +962,10 @@ function helpers.is_decimal_supported() return crud_utils.tarantool_supports_decimals() end +function helpers.is_uuid_supported() + return crud_utils.tarantool_supports_uuids() +end + function helpers.is_datetime_supported() return crud_utils.tarantool_supports_datetimes() end diff --git a/test/integration/count_test.lua b/test/integration/count_test.lua index dfbce6a1..6d1668de 100644 --- a/test/integration/count_test.lua +++ b/test/integration/count_test.lua @@ -893,3 +893,11 @@ for case_name_template, case in pairs(gh_373_types_cases) do case(g, read_impl) end end + +for case_name_template, case in pairs(read_scenario.gh_422_nullability_cases) do + local case_name = 'test_' .. case_name_template:format('count') + + pgroup[case_name] = function(g) + case(g, read_impl) + end +end diff --git a/test/integration/pairs_readview_test.lua b/test/integration/pairs_readview_test.lua index 3dcdb4cf..4577bc98 100644 --- a/test/integration/pairs_readview_test.lua +++ b/test/integration/pairs_readview_test.lua @@ -936,3 +936,11 @@ pgroup.after_test( 'test_pairs_merger_process_storage_error', read_scenario.after_merger_process_storage_error ) + +for case_name_template, case in pairs(read_scenario.gh_422_nullability_cases) do + local case_name = 'test_' .. case_name_template:format('pairs') + + pgroup[case_name] = function(g) + case(g, read_impl) + end +end diff --git a/test/integration/pairs_test.lua b/test/integration/pairs_test.lua index fd1f81e6..d5f850ba 100644 --- a/test/integration/pairs_test.lua +++ b/test/integration/pairs_test.lua @@ -944,3 +944,11 @@ pgroup.after_test( 'test_pairs_merger_process_storage_error', read_scenario.after_merger_process_storage_error ) + +for case_name_template, case in pairs(read_scenario.gh_422_nullability_cases) do + local case_name = 'test_' .. case_name_template:format('pairs') + + pgroup[case_name] = function(g) + case(g, read_impl) + end +end diff --git a/test/integration/read_scenario.lua b/test/integration/read_scenario.lua index 667bfb31..75a21e0d 100644 --- a/test/integration/read_scenario.lua +++ b/test/integration/read_scenario.lua @@ -5,9 +5,11 @@ -- Scenarios here are for `srv_select` entrypoint. local t = require('luatest') +local checks = require('checks') local _, datetime = pcall(require, 'datetime') local _, decimal = pcall(require, 'decimal') +local _, uuid = pcall(require, 'uuid') local helpers = require('test.helper') @@ -656,6 +658,330 @@ local function after_merger_process_storage_error(cg) end) end + +local sample = { + number = 1, +} + +if helpers.is_decimal_supported() then + sample.decimal = decimal.new('1.234') +end + +if helpers.is_uuid_supported() then + sample.uuid = uuid.fromstr('b5ed8123-8685-479b-93c0-021cccd2608e') +end + +if helpers.is_datetime_supported() then + sample.datetime = datetime.new{year = 2024, month = 3, day = 15} +end + +if helpers.is_interval_supported() then + sample.interval = datetime.interval.new{hour = -3} +end + +local function inherit(self, object) + setmetatable(object, self) + self.__index = self + return object +end + +local SimpleStorage = {inherit = inherit} + +local function space_string_repr(field_type, field_indexed, field_nullable) + -- Example: nonindexed_nullable_number_space + + local indexing_repr = field_indexed and 'indexed' or 'nonindexed' + local nullability_repr = field_nullable and 'nullable' or 'nonnullable' + + return ('%s_%s_%s_space'):format( + indexing_repr, + nullability_repr, + field_type + ) +end + +function SimpleStorage:new(opts) + checks('table', { + field_type = 'string', + field_nullable = 'boolean', + field_indexed = 'boolean', + }) + + local object = table.deepcopy(opts) + + -- Example: simple_number_nonindexed_nullable_nil + local space_name = space_string_repr(opts.field_type, opts.field_indexed, opts.field_indexed) + + local space_cfg = {space_name, opts.field_type, opts.field_indexed, opts.field_nullable} + + local data = {{id = 1, field = sample[opts.field_type]}} + if opts.field_nullable then + -- For Tarantool, box.NULL and nil fields are indistinguishable in comparison. + table.insert(data, {id = 2, field = box.NULL}) + end + + object.space_name = space_name + object._space_cfg = space_cfg + object._data = data + object._space_on_storage_prepared = {} + + self:inherit(object) + return object +end + +function SimpleStorage:prepare_space_on_storages(cg) + if self._space_on_storage_prepared[cg] then + return + end + + local function init_space_on_storage(space_name, field_type, field_indexed, field_nullable) + if box.info.ro == true then + return + end + + local engine = os.getenv('ENGINE') or 'memtx' + + local space = box.schema.space.create(space_name, { + if_not_exists = true, + engine = engine, + }) + + space:format({ + {name = 'id', type = 'unsigned'}, + {name = 'bucket_id', type = 'unsigned'}, + {name = 'field', type = field_type, is_nullable = field_nullable}, + }) + + space:create_index('pk', { + parts = { 'id' }, + if_not_exists = true, + }) + + space:create_index('bucket_id', { + parts = { 'bucket_id' }, + unique = false, + if_not_exists = true, + }) + + if field_indexed then + space:create_index('field_index', { + parts = { 'field' }, + unique = false, + if_not_exists = true, + }) + end + end + + helpers.call_on_storages(cg.cluster, function(server) + server:exec(init_space_on_storage, self._space_cfg) + end) + + helpers.insert_objects(cg, self.space_name, self:get_all_records()) + + self._space_on_storage_prepared[cg] = true +end + +function SimpleStorage:get_all_records() + return self._data +end + +function SimpleStorage:get_none_records() -- luacheck: ignore + return {} +end + +function SimpleStorage:get_all_null_records() + local resp = {} + for _, v in ipairs(self._data) do + if v['field'] == nil then + table.insert(resp, v) + end + end + + return resp +end + +function SimpleStorage:get_all_nonnull_records() + local resp = {} + for _, v in ipairs(self._data) do + if v['field'] ~= nil then + table.insert(resp, v) + end + end + + return resp +end + +local gh_422_nullability_space_cases = {} + +local types_for_simple_spaces = {'number'} + +if helpers.is_decimal_supported() then + table.insert(types_for_simple_spaces, 'decimal') +end + +if helpers.is_uuid_supported() then + table.insert(types_for_simple_spaces, 'uuid') +end + +if helpers.is_datetime_supported() then + table.insert(types_for_simple_spaces, 'datetime') +end + +if helpers.is_interval_supported() then + table.insert(types_for_simple_spaces, 'interval') +end + +for _, field_type in ipairs(types_for_simple_spaces) do + local indexing_cases = {false, true} + if field_type == 'interval' then + indexing_cases = {false} + end + + for _, field_indexed in ipairs(indexing_cases) do + local nullability_cases = {false, true} + + for _, field_nullable in ipairs(nullability_cases) do + local storage = SimpleStorage:new{ + field_type = field_type, + field_indexed = field_indexed, + field_nullable = field_nullable, + } + gh_422_nullability_space_cases[storage.space_name] = storage + end + end +end + +local gh_422_nullability_condition_cases = {} + +local function get_conditions(operator, operand, opts) + checks('string', '?', {secondary_condition = 'boolean'}) + + local null_condition = {operator, 'field', operand} + + local conditions + if opts.secondary_condition then + conditions = {{'>=', 'id', 1}, null_condition} + else + conditions = {null_condition} + end + + return conditions +end + +local operator_cases = { + -- The logic in core Tarantool select is as follows. + -- nil condition is an absence of condition, thus all data is returned + -- disregarding the condition (condition may affect the order). + -- box.NULL condition is a condition for the null value -- in case of EQ ('=='), + -- only records with null index value are returned, in case of GT ('>') + -- all non-null values are returned since nulls are in the beginning of an index + -- and so on. Nils and box.NULLs in tuple are both satisfy box.NULL condition. + lt_condition_with_nil_operand = { + operator = '<', + operand = nil, + expected_objects_getter = 'get_all_records', + }, + le_condition_with_nil_operand = { + operator = '<=', + operand = nil, + expected_objects_getter = 'get_all_records', + }, + eq_condition_with_nil_operand = { + operator = '==', + operand = nil, + expected_objects_getter = 'get_all_records', + }, + ge_condition_with_nil_operand = { + operator = '>=', + operand = nil, + expected_objects_getter = 'get_all_records', + }, + gt_condition_with_nil_operand = { + operator = '>', + operand = nil, + expected_objects_getter = 'get_all_records', + }, + lt_condition_with_boxNULL_operand = { + operator = '<', + operand = box.NULL, + expected_objects_getter = 'get_none_records', + }, + le_condition_with_boxNULL_operand = { + operator = '<=', + operand = box.NULL, + expected_objects_getter = 'get_all_null_records', + }, + eq_condition_with_boxNULL_operand = { + operator = '==', + operand = box.NULL, + expected_objects_getter = 'get_all_null_records', + }, + ge_condition_with_boxNULL_operand = { + operator = '>=', + operand = box.NULL, + expected_objects_getter = 'get_all_records', + }, + gt_condition_with_boxNULL_operand = { + operator = '>', + operand = box.NULL, + expected_objects_getter = 'get_all_nonnull_records', + }, +} + +local secondarity_cases = {false, true} +for _, secondary_condition in ipairs(secondarity_cases) do + local secondarity_string_repr = secondary_condition and 'second' or 'single' + + for op_case_name, op_case in pairs(operator_cases) do + local case_name = ('%s_%s'):format(secondarity_string_repr, op_case_name) + + local conditions = get_conditions( + op_case.operator, + op_case.operand, + {secondary_condition = secondary_condition} + ) + + gh_422_nullability_condition_cases[case_name] = { + conditions = conditions, + expected_objects_getter = op_case.expected_objects_getter, + } + end +end + +local gh_422_nullability_cases = {} + +for space_case_name, space_case in pairs(gh_422_nullability_space_cases) do + for condition_case_name, condition_case in pairs(gh_422_nullability_condition_cases) do + local case_name_template = ('gh_422_%%s_%s_with_%s'):format(space_case_name, condition_case_name) + + local function case(cg, read) + -- Skip not needed since unsupported cases are not generated. + space_case:prepare_space_on_storages(cg) + + local result, err = read(cg, space_case.space_name, condition_case.conditions) + t.assert_equals(err, nil) + + -- Order may vary depending on indexes and conditions. + local getter = condition_case.expected_objects_getter + local expected_objects_without_bucket_id = space_case[getter](space_case) + + if type(result) == 'number' then -- crud.count + t.assert_equals(result, #expected_objects_without_bucket_id) + else + local actual_objects_without_bucket_id = {} + for k, v in pairs(result) do + v['bucket_id'] = nil + actual_objects_without_bucket_id[k] = v + end + + t.assert_items_equals(actual_objects_without_bucket_id, expected_objects_without_bucket_id) + end + end + + gh_422_nullability_cases[case_name_template] = case + end +end + return { gh_418_read_with_secondary_noneq_index_condition = gh_418_read_with_secondary_noneq_index_condition, gh_373_read_with_decimal_condition_cases = gh_373_read_with_decimal_condition_cases, @@ -664,4 +990,5 @@ return { before_merger_process_storage_error = before_merger_process_storage_error, merger_process_storage_error = merger_process_storage_error, after_merger_process_storage_error = after_merger_process_storage_error, + gh_422_nullability_cases = gh_422_nullability_cases, } diff --git a/test/integration/select_readview_test.lua b/test/integration/select_readview_test.lua index cb3718ef..87a5b7d6 100644 --- a/test/integration/select_readview_test.lua +++ b/test/integration/select_readview_test.lua @@ -2545,3 +2545,11 @@ pgroup.after_test( 'test_select_merger_process_storage_error', read_scenario.after_merger_process_storage_error ) + +for case_name_template, case in pairs(read_scenario.gh_422_nullability_cases) do + local case_name = 'test_' .. case_name_template:format('select') + + pgroup[case_name] = function(g) + case(g, read_impl) + end +end diff --git a/test/integration/select_test.lua b/test/integration/select_test.lua index fed8064d..70b834e9 100644 --- a/test/integration/select_test.lua +++ b/test/integration/select_test.lua @@ -2313,3 +2313,11 @@ pgroup.after_test( 'test_select_merger_process_storage_error', read_scenario.after_merger_process_storage_error ) + +for case_name_template, case in pairs(read_scenario.gh_422_nullability_cases) do + local case_name = 'test_' .. case_name_template:format('select') + + pgroup[case_name] = function(g) + case(g, read_impl) + end +end