From 10a65c7e584820b403567bc849581bb0c3176ff5 Mon Sep 17 00:00:00 2001 From: Sebastian Harl Date: Fri, 17 Oct 2014 19:50:34 +0200 Subject: [PATCH] store, frontend: Make IS (NOT) NULL an unary operator on expressions. This is more flexible and in-line with all other operators being migrated to expression-based constructs. --- src/core/store-private.h | 4 +- src/core/store_lookup.c | 65 ++++++++++++++++++++------------- src/frontend/grammar.y | 37 +++++++++++++++---- src/include/core/store.h | 11 +++++- t/unit/core/store_lookup_test.c | 12 +++--- t/unit/frontend/parser_test.c | 2 +- 6 files changed, 87 insertions(+), 44 deletions(-) diff --git a/src/core/store-private.h b/src/core/store-private.h index a64c220..3db771b 100644 --- a/src/core/store-private.h +++ b/src/core/store-private.h @@ -181,6 +181,7 @@ enum { MATCHER_REGEX, MATCHER_NREGEX, MATCHER_ISNULL, + MATCHER_ISNNULL, }; #define MATCHER_SYM(t) \ @@ -201,6 +202,7 @@ enum { : ((t) == MATCHER_REGEX) ? "=~" \ : ((t) == MATCHER_NREGEX) ? "!~" \ : ((t) == MATCHER_ISNULL) ? "IS NULL" \ + : ((t) == MATCHER_ISNNULL) ? "IS NOT NULL" \ : "UNKNOWN") /* match the name of something */ @@ -272,7 +274,7 @@ typedef struct { typedef struct { sdb_store_matcher_t super; - char *attr_name; /* we only support matching attributes */ + sdb_store_expr_t *expr; } isnull_matcher_t; #define ISNULL_M(m) ((isnull_matcher_t *)(m)) diff --git a/src/core/store_lookup.c b/src/core/store_lookup.c index 325d67f..21e0ba4 100644 --- a/src/core/store_lookup.c +++ b/src/core/store_lookup.c @@ -528,10 +528,23 @@ static int match_isnull(sdb_store_matcher_t *m, sdb_store_obj_t *obj, sdb_store_matcher_t *filter) { - assert(m->type == MATCHER_ISNULL); - if (obj->type != SDB_HOST) - return 0; - return attr_get(HOST(obj), ISNULL_M(m)->attr_name, filter) == NULL; + sdb_data_t v = SDB_DATA_INIT; + int status; + + assert((m->type == MATCHER_ISNULL) || (m->type == MATCHER_ISNNULL)); + + /* TODO: this might hide real errors; + * improve error reporting and propagation */ + if (sdb_store_expr_eval(ISNULL_M(m)->expr, obj, &v, filter) + || sdb_data_isnull(&v)) + status = 1; + else + status = 0; + + sdb_data_free_datum(&v); + if (m->type == MATCHER_ISNNULL) + return !status; + return status; } /* match_isnull */ typedef int (*matcher_cb)(sdb_store_matcher_t *, sdb_store_obj_t *, @@ -563,6 +576,7 @@ matchers[] = { match_regex, match_regex, match_isnull, + match_isnull, }; /* @@ -950,33 +964,30 @@ uop_tostring(sdb_store_matcher_t *m, char *buf, size_t buflen) static int isnull_matcher_init(sdb_object_t *obj, va_list ap) { - const char *name; - M(obj)->type = va_arg(ap, int); - if (M(obj)->type != MATCHER_ISNULL) + if ((M(obj)->type != MATCHER_ISNULL) && (M(obj)->type != MATCHER_ISNNULL)) return -1; - name = va_arg(ap, const char *); - if (! name) - return -1; - ISNULL_M(obj)->attr_name = strdup(name); - if (! ISNULL_M(obj)->attr_name) - return -1; + ISNULL_M(obj)->expr = va_arg(ap, sdb_store_expr_t *); + sdb_object_ref(SDB_OBJ(ISNULL_M(obj)->expr)); return 0; } /* isnull_matcher_init */ static void isnull_matcher_destroy(sdb_object_t *obj) { - if (ISNULL_M(obj)->attr_name) - free(ISNULL_M(obj)->attr_name); - ISNULL_M(obj)->attr_name = NULL; + sdb_object_deref(SDB_OBJ(ISNULL_M(obj)->expr)); + ISNULL_M(obj)->expr = NULL; } /* isnull_matcher_destroy */ static char * isnull_tostring(sdb_store_matcher_t *m, char *buf, size_t buflen) { - snprintf(buf, buflen, "(IS NULL, ATTR[%s])", ISNULL_M(m)->attr_name); + /* XXX */ + if (m->type == MATCHER_ISNULL) + strncpy(buf, "(IS NULL)", buflen); + else + strncpy(buf, "(IS NOT NULL)", buflen); return buf; } /* isnull_tostring */ @@ -1056,6 +1067,7 @@ matchers_tostring[] = { cmp_tostring, cmp_tostring, isnull_tostring, + isnull_tostring, }; /* @@ -1236,12 +1248,19 @@ sdb_store_nregex_matcher(sdb_store_expr_t *left, sdb_store_expr_t *right) } /* sdb_store_nregex_matcher */ sdb_store_matcher_t * -sdb_store_isnull_matcher(const char *attr_name) +sdb_store_isnull_matcher(sdb_store_expr_t *expr) { return M(sdb_object_create("isnull-matcher", isnull_type, - MATCHER_ISNULL, attr_name)); + MATCHER_ISNULL, expr)); } /* sdb_store_isnull_matcher */ +sdb_store_matcher_t * +sdb_store_isnnull_matcher(sdb_store_expr_t *expr) +{ + return M(sdb_object_create("isnull-matcher", isnull_type, + MATCHER_ISNNULL, expr)); +} /* sdb_store_isnnull_matcher */ + sdb_store_matcher_op_cb sdb_store_parse_matcher_op(const char *op) { @@ -1341,13 +1360,7 @@ parse_attr_cmp(const char *attr, const char *op, sdb_store_expr_t *expr) if (! attr) return NULL; - if (! strcasecmp(op, "IS")) { - if (! expr) - return sdb_store_isnull_matcher(attr); - else - return NULL; - } - else if (! expr) + if (! expr) return NULL; else if (parse_cond_op(op, &matcher, &inv)) return NULL; diff --git a/src/frontend/grammar.y b/src/frontend/grammar.y index 1926718..24a1375 100644 --- a/src/frontend/grammar.y +++ b/src/frontend/grammar.y @@ -460,21 +460,44 @@ compare_matcher: | IDENTIFIER '[' IDENTIFIER ']' IS NULL_T { - $$ = sdb_store_matcher_parse_cmp($1, $3, "IS", NULL); + sdb_store_expr_t *expr; + + if (strcasecmp($1, "attribute")) { + char errmsg[strlen($1) + strlen($3) + 32]; + snprintf(errmsg, sizeof(errmsg), + YY_("unknown value %s[%s]"), $1); + sdb_fe_yyerror(&yylloc, scanner, errmsg); + free($1); $1 = NULL; + free($3); $3 = NULL; + YYABORT; + } + + expr = sdb_store_expr_attrvalue($3); + $$ = sdb_store_isnull_matcher(expr); + sdb_object_deref(SDB_OBJ(expr)); free($1); $1 = NULL; free($3); $3 = NULL; } | IDENTIFIER '[' IDENTIFIER ']' IS NOT NULL_T { - sdb_store_matcher_t *m; - m = sdb_store_matcher_parse_cmp($1, $3, "IS", NULL); + sdb_store_expr_t *expr; + + if (strcasecmp($1, "attribute")) { + char errmsg[strlen($1) + strlen($3) + 32]; + snprintf(errmsg, sizeof(errmsg), + YY_("unknown value %s[%s]"), $1); + sdb_fe_yyerror(&yylloc, scanner, errmsg); + free($1); $1 = NULL; + free($3); $3 = NULL; + YYABORT; + } + + expr = sdb_store_expr_attrvalue($3); + $$ = sdb_store_isnnull_matcher(expr); + sdb_object_deref(SDB_OBJ(expr)); free($1); $1 = NULL; free($3); $3 = NULL; - - /* sdb_store_inv_matcher return NULL if m==NULL */ - $$ = sdb_store_inv_matcher(m); - sdb_object_deref(SDB_OBJ(m)); } ; diff --git a/src/include/core/store.h b/src/include/core/store.h index 147d745..a7804dd 100644 --- a/src/include/core/store.h +++ b/src/include/core/store.h @@ -406,10 +406,17 @@ sdb_store_attr_matcher(const char *name, const char *value, _Bool re); /* * sdb_store_isnull_matcher: - * Creates a matcher matching "missing" attributes. + * Creates a matcher matching NULL values. */ sdb_store_matcher_t * -sdb_store_isnull_matcher(const char *attr_name); +sdb_store_isnull_matcher(sdb_store_expr_t *expr); + +/* + * sdb_store_isnnull_matcher: + * Creates a matcher matching non-NULL values. + */ +sdb_store_matcher_t * +sdb_store_isnnull_matcher(sdb_store_expr_t *expr); /* * sdb_store_child_matcher: diff --git a/t/unit/core/store_lookup_test.c b/t/unit/core/store_lookup_test.c index 3b0e956..2e1e6d8 100644 --- a/t/unit/core/store_lookup_test.c +++ b/t/unit/core/store_lookup_test.c @@ -624,8 +624,6 @@ START_TEST(test_parse_cmp) /* { "attribute", "attr", "=", &attrname, MATCHER_EQ }, */ { "attribute", "attr", ">=", &attrname, MATCHER_GE }, { "attribute", "attr", ">", &attrname, MATCHER_GT }, - { "attribute", "attr", "IS", NULL, MATCHER_ISNULL }, - { "attribute", "attr", "IS", &attrname, -1 }, { "foo", NULL, "=", &attrname, -1 }, { "foo", "attr", "=", &attrname, -1 }, }; @@ -657,7 +655,7 @@ START_TEST(test_parse_cmp) fail_unless(check != NULL, "sdb_store_matcher_parse_cmp(%s, %s, %s, %s) = %p; " - "expected: NULL", golden_data[i].obj_type, + "expected: ", golden_data[i].obj_type, golden_data[i].attr, golden_data[i].op, buf, check); fail_unless(M(check)->type == golden_data[i].expected, "sdb_store_matcher_parse_cmp(%s, %s, %s, %s) returned matcher " @@ -834,13 +832,13 @@ START_TEST(test_scan) { "attribute[k1] = 'v1'", NULL, 1, "ATTR\\[k1\\]\\{ VALUE\\{ 'v1', \\(nil\\) \\} \\}" }, { "attribute[k1] IS NULL", NULL, 2, - "\\(IS NULL, ATTR\\[k1\\]\\)" }, + "\\(IS NULL\\)" }, { "attribute[x1] IS NULL", NULL, 3, - "\\(IS NULL, ATTR\\[x1\\]\\)" }, + "\\(IS NULL\\)" }, { "attribute[k1] IS NOT NULL", NULL, 1, - "\\(NOT, \\(IS NULL, ATTR\\[k1\\]\\)\\)" }, + "\\(IS NOT NULL\\)" }, { "attribute[x1] IS NOT NULL", NULL, 0, - "\\(NOT, \\(IS NULL, ATTR\\[x1\\]\\)\\)" }, + "\\(IS NOT NULL\\)" }, { "attribute[k2] < 123", NULL, 0, "ATTR\\[k2\\]\\{ < 123 \\}" }, { "attribute[k2] <= 123", NULL, 1, diff --git a/t/unit/frontend/parser_test.c b/t/unit/frontend/parser_test.c index c0cbff8..7839ee8 100644 --- a/t/unit/frontend/parser_test.c +++ b/t/unit/frontend/parser_test.c @@ -348,7 +348,7 @@ START_TEST(test_parse_matcher) /* NULL; while this is an implementation detail, * IS NULL currently maps to an equality matcher */ { "attribute[foo] IS NULL", -1, MATCHER_ISNULL }, - { "attribute[foo] IS NOT NULL", -1, MATCHER_NOT }, + { "attribute[foo] IS NOT NULL", -1, MATCHER_ISNNULL }, /* object field matchers */ { ".last_update < 10s", -1, MATCHER_LT }, -- 2.30.2