Add a JSON extract test that extracts row data #1066

waynelwarren · 2021-11-05T23:54:09Z

This is a different test file than test_gaia_db_extract.cpp because it uses one of the test DDL files, which is required in order to insert rows with direct access.

The test exercises the gaia_db_extract function in new ways with a variety of parameters.

The CMakeLists.txt file was updated according to a pattern found in other tests.

This has test already founds bugs, which have been fixed in the gaia_db_extract.cpp file.

production/tools/gaia_db_extract/src/gaia_db_extract.cpp

LaurentiuCristofor · 2021-11-06T02:16:20Z

production/tools/gaia_db_extract/src/gaia_db_extract.cpp

    auto field_name = field_object.name();
    switch (value.type)
    {
    case reflection::String:
-        row[field_name] = value.hold.string_value;
+        // Null is possible.
+        row[field_name] = value.hold.string_value == nullptr ? s_null_string : value.hold.string_value;


Enclose the expression in parentheses - i.e.:

row[field_name] = (value.hold.string_value == nullptr) ? s_null_string : value.hold.string_value;

I'm surprised that clang didn't ask for this.

LaurentiuCristofor · 2021-11-06T02:17:32Z

production/tools/gaia_db_extract/src/gaia_db_extract.cpp

@@ -192,14 +196,15 @@ static bool add_field_value(json_t& row, const gaia_field_t& field_object, const
 static string dump_rows(string database, string table, uint64_t start_after, uint32_t row_limit)
 {
    bool terminate = false;
+    bool top_level_included = false;


Start the boolean names with a verb. This should be include_top_level instead.

Or it sounds like has_included_top_level is actually the better form.

LaurentiuCristofor · 2021-11-06T02:21:09Z

production/tools/gaia_db_extract/tests/test_row_extract.cpp

+    // Try this with a number of permutations.
+    test_with_different_sizes(100, 3);
+
+    // It is important to run this with > 1024 rows.


You could define a min constant, set it to 1024 and pass values based on it to the tests.

I think I did what you suggested on all comments (so far).

Actually if you're referring to the strange iterator behavior you observed, you want the constant db_server_t::c_stream_batch_size. That's private for now though.

BTW I just duplicated that constant in production/db/core/tests/test_db_client.cpp:

// duplicated from production/db/core/inc/db_server.hpp constexpr size_t c_stream_batch_size = 1 << 10;

Not sure if duplication or loss of encapsulation is the lesser evil here.

I'm not sure why this test should be aware of such a constant. The discovery of the issue had nothing to do with internal knowledge.

senderista · 2021-11-09T16:38:56Z

Just a nit: I think there's a bit of confusion re: the meaning of *PRIVATE_INCLUDES vs. *PUBLIC_INCLUDES. The intention was to specify whether this target's include directories are inherited by other targets depending on this target via target_link_libraries() (the PUBLIC keyword forces inheritance). It doesn't have anything to do with whether the include directories are in a location that we consider "public" (i.e. part of the SDK) or "private" (i.e. internal). For an executable, no other targets will ever depend on this target, so the PUBLIC/PRIVATE include dir distinction is unnecessary (everything can be PRIVATE).

JackAtGaia · 2021-11-09T18:27:05Z

production/tools/gaia_db_extract/src/gaia_db_extract.cpp

@@ -158,7 +158,8 @@ static bool add_field_value(json_t& row, const gaia_field_t& field_object, const
    switch (value.type)
    {
    case reflection::String:
-        row[field_name] = value.hold.string_value;
+        // Null is possible.
+        row[field_name] = (value.hold.string_value == nullptr) ? c_empty_string : value.hold.string_value;


did clang-tidy ask for this?

No, but Laurentiu thought that it should.

JackAtGaia · 2021-11-09T18:28:11Z

production/tools/gaia_db_extract/tests/test_row_extract.cpp

+    // Utility function that creates one named employee row.
+    employee_t create_employee(const char* name)
+    {
+        auto w = employee_writer();


w should be employee_w according to standards.

And I'd change e->employee below as well.

Okay @LaurentiuCristofor , did that too.

JackAtGaia · 2021-11-09T18:28:50Z

production/tools/gaia_db_extract/tests/test_row_extract.cpp

+    {
+        begin_transaction();
+
+#ifdef GAIAPLAT_1673


this probably should have a comment talking about why

Added a comment.

JackAtGaia · 2021-11-09T18:30:38Z

production/tools/gaia_db_extract/tests/test_row_extract.cpp

+        return e;
+    }
+
+    void test_with_different_sizes(uint32_t number_rows_inserted, uint32_t block_size)


test methods should have comments that very clearly document what it is testing and what the expected result is.

without this information, I cannot figure out what this test is for.

I have commented all of the test functions, including the new TEST_F().

JackAtGaia · 2021-11-09T18:31:50Z

production/tools/gaia_db_extract/tests/test_row_extract.cpp

+};
+
+// Store rows, convert them to JSON, scan through them as multiple blocks.
+TEST_F(row_extract_test, read_blocks)


if this is the actual test, why is it calling a function/method that has a name starting with test_?

I simply named the function after what it does. the Actual test has a number of sub-tests.

waynelwarren · 2021-11-10T18:44:24Z

I just added an entirely new test here - it does the full cycle of insert_row(), extract as JSON string, parse from JSON string, compare JSON values to original database values. If you can, @JackAtGaia and @LaurentiuCristofor , I would appreciate your review.

LaurentiuCristofor

Left some minor comments, LGTM otherwise.

LaurentiuCristofor · 2021-11-10T19:07:33Z

production/tools/gaia_db_extract/tests/test_row_extract.cpp

+        // Create a bunch of employee rows.
+        for (uint32_t i = 0; i < number_rows_inserted; i++)
+        {
+            string empl_name = "Mr. " + std::to_string(i);


empl_name could just be reduced to name here.

LaurentiuCristofor · 2021-11-10T19:08:32Z

production/tools/gaia_db_extract/tests/test_row_extract.cpp

+        uint64_t row_id = c_start_at_first;
+        int32_t row_count = 0;
+        int32_t block_count = 0;
+        for (;;)


It's more typical to use while (true) for such loops.

LaurentiuCristofor · 2021-11-10T19:09:16Z

production/tools/gaia_db_extract/tests/test_row_extract.cpp

+                ++row_count;
+            }
+        }
+        if (block_size > 0)


I'd add a line to separate this if from the previous loop.

Easy enough, I have made the changes. See them in the next commit.

In a previous life, for (;;) was a common convention. I think both forms are clear.

waynelwarren added 3 commits November 5, 2021 16:00

add a JSON test to scan blocks of employee_t rows

e786617

turn the empty object {} into a constant

d03708b

merge with master

a07be1f

waynelwarren requested review from LaurentiuCristofor, simone-gaia and JackAtGaia November 5, 2021 23:54

LaurentiuCristofor reviewed Nov 5, 2021

View reviewed changes

production/tools/gaia_db_extract/src/gaia_db_extract.cpp Outdated Show resolved Hide resolved

LaurentiuCristofor reviewed Nov 6, 2021

View reviewed changes

waynelwarren added 4 commits November 6, 2021 10:40

use existing constant for empty string

d7e81aa

using constants in tests, other PR responses

375c4ea

include GAIAPLAT-1673 section of code

294b854

Merge branch 'master' into wayne/add-json-extract-test

ce4ecba

JackAtGaia reviewed Nov 9, 2021

View reviewed changes

JackAtGaia suggested changes Nov 9, 2021

View reviewed changes

waynelwarren added 2 commits November 9, 2021 12:31

respond to PR comments

2ec9f0c

adding a second JSON test, comparing stored/extracted field values

1499093

changed 'e' to 'employee' per convention

a87fa73

LaurentiuCristofor approved these changes Nov 10, 2021

View reviewed changes

waynelwarren added 2 commits November 12, 2021 11:33

two small edits respoinding to PR comments

0455b03

Merge branch 'master' into wayne/add-json-extract-test

da886b6

JackAtGaia approved these changes Nov 17, 2021

View reviewed changes

Merge branch 'master' into wayne/add-json-extract-test

af23619

waynelwarren merged commit ae328d9 into master Nov 17, 2021

waynelwarren deleted the wayne/add-json-extract-test branch November 17, 2021 16:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a JSON extract test that extracts row data #1066

Add a JSON extract test that extracts row data #1066

waynelwarren commented Nov 5, 2021

LaurentiuCristofor Nov 6, 2021

LaurentiuCristofor Nov 6, 2021

LaurentiuCristofor Nov 6, 2021

LaurentiuCristofor Nov 6, 2021 •

edited

Loading

waynelwarren Nov 8, 2021

senderista Nov 9, 2021

senderista Nov 9, 2021

waynelwarren Nov 10, 2021

senderista commented Nov 9, 2021 •

edited

Loading

JackAtGaia Nov 9, 2021

waynelwarren Nov 9, 2021

JackAtGaia Nov 9, 2021

LaurentiuCristofor Nov 9, 2021

waynelwarren Nov 9, 2021

waynelwarren Nov 10, 2021

JackAtGaia Nov 9, 2021

waynelwarren Nov 9, 2021

JackAtGaia Nov 9, 2021

waynelwarren Nov 10, 2021

JackAtGaia Nov 9, 2021

waynelwarren Nov 9, 2021

waynelwarren commented Nov 10, 2021

LaurentiuCristofor left a comment

LaurentiuCristofor Nov 10, 2021

LaurentiuCristofor Nov 10, 2021

LaurentiuCristofor Nov 10, 2021

waynelwarren Nov 10, 2021

Add a JSON extract test that extracts row data #1066

Add a JSON extract test that extracts row data #1066

Conversation

waynelwarren commented Nov 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LaurentiuCristofor Nov 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

senderista commented Nov 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waynelwarren commented Nov 10, 2021

LaurentiuCristofor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LaurentiuCristofor Nov 6, 2021 •

edited

Loading

senderista commented Nov 9, 2021 •

edited

Loading