Improve command-line processing of gaia_db_extract #1048

waynelwarren · 2021-10-28T19:22:07Z

General improvements to the main() function of gaia_db_extract.

Addresses https://gaiaplatform.atlassian.net/browse/GAIAPLAT-1630.

Improved the robustness and compactness of the command-line processing. Added usage() for invalid command-line options.

production/tools/gaia_db_extract/src/main.cpp

simone-gaia · 2021-10-29T17:46:56Z

There are a bunch of warnings comeing from this code:

/home/simone/repos/GaiaPlatform/production/tools/gaia_db_extract/inc/table_iterator.hpp:33:27: warning: 2 uninitialized fields at the end of the constructor call [clang-analyzer-optin.cplusplus.UninitializedObject]
        m_current_payload = nullptr;
                          ^
/home/simone/repos/GaiaPlatform/production/tools/gaia_db_extract/inc/table_iterator.hpp:64:11: note: uninitialized pointer 'this->m_table_name'
    char* m_table_name;
          ^
/home/simone/repos/GaiaPlatform/production/tools/gaia_db_extract/inc/table_iterator.hpp:67:31: note: uninitialized field 'this->m_container_id'
    gaia::common::gaia_type_t m_container_id;
                              ^
/home/simone/repos/GaiaPlatform/production/tools/gaia_db_extract/src/gaia_db_extract.cpp:274:9: note: Assuming the condition is false
    if (database.size() == 0 && table.size() == 0)
        ^
/home/simone/repos/GaiaPlatform/production/tools/gaia_db_extract/src/gaia_db_extract.cpp:274:30: note: Left side of '&&' is false
    if (database.size() == 0 && table.size() == 0)
                             ^
/home/simone/repos/GaiaPlatform/production/tools/gaia_db_extract/src/gaia_db_extract.cpp:278:14: note: Assuming the condition is true
    else if (database.size() > 0 && table.size() > 0)
             ^
/home/simone/repos/GaiaPlatform/production/tools/gaia_db_extract/src/gaia_db_extract.cpp:278:14: note: Left side of '&&' is true
/home/simone/repos/GaiaPlatform/production/tools/gaia_db_extract/src/gaia_db_extract.cpp:278:37: note: Assuming the condition is true
    else if (database.size() > 0 && table.size() > 0)
                                    ^
/home/simone/repos/GaiaPlatform/production/tools/gaia_db_extract/src/gaia_db_extract.cpp:278:10: note: Taking true branch
    else if (database.size() > 0 && table.size() > 0)
         ^
/home/simone/repos/GaiaPlatform/production/tools/gaia_db_extract/src/gaia_db_extract.cpp:280:16: note: Calling 'dump_rows'
        return dump_rows(database, table, start_after, row_limit);
               ^
/home/simone/repos/GaiaPlatform/production/tools/gaia_db_extract/src/gaia_db_extract.cpp:195:22: note: Calling default constructor for 'table_iterator_t'
    table_iterator_t table_iterator;
                     ^
/home/simone/repos/GaiaPlatform/production/tools/gaia_db_extract/inc/table_iterator.hpp:33:27: note: 2 uninitialized fields at the end of the constructor call
        m_current_payload = nullptr;
                          ^
/home/simone/repos/GaiaPlatform/production/tools/gaia_db_extract/src/gaia_db_extract.cpp:65:15: warning: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy]
    for (auto field : table.gaia_fields())
              ^
         const  &
/home/simone/repos/GaiaPlatform/production/tools/gaia_db_extract/src/gaia_db_extract.cpp:80:15: warning: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy]
    for (auto table : db.gaia_tables())
              ^
         const  &
/home/simone/repos/GaiaPlatform/production/tools/gaia_db_extract/src/gaia_db_extract.cpp:89:13: warning: invalid case style for global variable 's_cache_initialized' [readability-identifier-naming]
static bool s_cache_initialized = false;
            ^~~~~~~~~~~~~~~~~~~
            g_s_cache_initialized
/home/simone/repos/GaiaPlatform/production/tools/gaia_db_extract/src/gaia_db_extract.cpp:127:15: warning: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy]
    for (auto db : gaia_database_t::list())
              ^
         const  &
/home/simone/repos/GaiaPlatform/production/tools/gaia_db_extract/src/table_iterator.cpp:98:22: warning: Value stored to 'value' during its initialization is never read [clang-analyzer-deadcode.DeadStores]
                auto value = get_field_array_element(
                     ^
/home/simone/repos/GaiaPlatform/production/tools/gaia_db_extract/src/table_iterator.cpp:98:22: note: Value stored to 'value' during its initialization is never read

daxhaw · 2021-10-29T23:23:53Z

production/tools/gaia_db_extract/src/main.cpp

+// Command-line usage.
+static void usage()
+{
+    fprintf(stderr, "Usage: gaia_db_extract [--database=<dbname>] [--table=<tableneme>] [--start-after=ID] [--row-limit=N]\n");


nit: you have defined constants for the arguments (except for row-limit). Why not use them here as well?

I made some changes to this. Hope this is what you meant.

daxhaw · 2021-10-29T23:24:33Z

production/tools/gaia_db_extract/src/main.cpp

+static void usage()
+{
+    fprintf(stderr, "Usage: gaia_db_extract [--database=<dbname>] [--table=<tableneme>] [--start-after=ID] [--row-limit=N]\n");
+    fprintf(stderr, "  No parameters: dump catalog.\n");


nit: why not just use std::cerr and the stream operators here? (more modern C++)

daxhaw · 2021-10-29T23:30:05Z

production/tools/gaia_db_extract/src/main.cpp

            {
-                row_limit = atoi(argv[++i]);
+                start_after = atoi(value.c_str());


nit: I think you can use std::stoi and just use the string value without needing to convert it to a C string via c_str().

Did this too.

daxhaw · 2021-10-29T23:31:09Z

production/tools/gaia_db_extract/src/main.cpp

+                start_after = atoi(value.c_str());
+                if (start_after < 1)
+                {
+                    fprintf(stderr, "Illegal value for start_after. It must be 1 or greater\n");


nit: you could use your constant here. If you use the stream operators << then this is something like:

std::cerr << "Illegal value for " << c_start_after << "...";

Okay, it's now done this way everywhere.

waynelwarren · 2021-11-02T16:57:42Z

@simone-gaia , I don't get those warnings when I build. And that file wasn't changed in this PR. What causes this to happen?

simone-gaia · 2021-11-02T17:22:10Z

@simone-gaia , I don't get those warnings when I build. And that file wasn't changed in this PR. What causes this to happen?

I noticed those warnings by doing a full build of the system. Even though you did not modify gaia_db_extract.cpp you could take the occasion to remove the warnings.

waynelwarren · 2021-11-02T20:00:31Z

@simone-gaia , I'm seeing the warnings too. The best way for me to address them is in a new PR.

simone-gaia · 2021-11-02T22:09:47Z

@simone-gaia , I'm seeing the warnings too. The best way for me to address them is in a new PR.

As you wish, just pointing out.

production/tools/gaia_db_extract/src/main.cpp

LaurentiuCristofor

Validate the error messages with @vDonGlover ; otherwise these changes look good to me.

vDonGlover · 2021-11-04T16:04:21Z

production/tools/gaia_db_extract/src/main.cpp

@@ -24,10 +24,10 @@ constexpr char c_table_string[] = "table";
 static void usage()
 {
    cerr << "Usage: gaia_db_extract [--" << c_database_string << "=<databasename>] [--" << c_table_string << "=<tableneme>] [--"
-         << c_start_string << "=ID] [--" << c_row_limit_string << "=N]" << endl;
+         << c_start_after_string << "=ID] [--" << c_row_limit_string << "=N]" << endl;
    cerr << "  No parameters: dump the catalog only." << endl;
    cerr << "  Else dump rows specified by " << c_database_string << "/" << c_table_string << " name, limited by "


It is unclear to me what is going on here. What is being reported? And is the message saying what it is doing or giving instructions of what to do?

vDonGlover · 2021-11-04T17:30:00Z

production/tools/gaia_db_extract/src/main.cpp

 {
    if (strncmp(argv[arg], "--", 2))
    {
-        fprintf(stderr, "Incorrect command-line parameter: %s\n", argv[arg]);
-        return false;
+        cerr << "Incorrect command-line parameter: " << argv[arg] << endl;


Do we have 'standard" wording for this? In gaiac we say
cerr << c_error_prompt << "Unrecognized parameter: " << argv[i] << endl;

Let's go with that.

vDonGlover · 2021-11-04T17:37:49Z

production/tools/gaia_db_extract/src/main.cpp

-    fprintf(stderr, "  Else dump rows specified by database/table name, limited by start-after and row-limit.\n");
+    cerr << "Usage: gaia_db_extract [--" << c_database_string << "=<databasename>] [--" << c_table_string << "=<tableneme>] [--"
+         << c_start_string << "=ID] [--" << c_row_limit_string << "=N]" << endl;
+    cerr << "  No parameters: dump the catalog only." << endl;


Does this mean "Extracts the entire catalog" (ie all of the contents of the database)? Or does it mean that it extracts the structure of the database? Or ...?

vDonGlover · 2021-11-04T17:55:35Z

production/tools/gaia_db_extract/src/main.cpp

+        }
+        else
+        {
+            cerr << "Invalid command-line option: '" << key << "'." << endl;


How does this differ from an invalid parameter?

It's not. Have made them the same.

waynelwarren · 2021-11-04T21:11:53Z

@vDonGlover , I have reworked the usage() and error messages significantly. Please review again.

vDonGlover · 2021-11-04T21:27:13Z

production/tools/gaia_db_extract/src/main.cpp

-    cerr << "  No parameters: dump the catalog only." << endl;
-    cerr << "  Else dump rows specified by " << c_database_string << "/" << c_table_string << " name, limited by "
-         << c_start_after_string << " and " << c_row_limit_string << "." << endl;
+    cerr << "OVERVIEW: Produce JSON representation of catalog, or database rows:" << endl;


Is this saying something like this:

OVERVIEW: Outputs a JSON representation of either the catalog structure or selected rows for a specified database and table within the database.

That's better, of course. I just like being terse.

vDonGlover · 2021-11-04T21:32:30Z

production/tools/gaia_db_extract/src/main.cpp

+    cerr << "  --" << c_database_string << "=<databasename>    Required. Selects the database from which to extract rows." << endl;
+    cerr << "  --" << c_table_string << "=<tablename>          Required. Selects the table containing the rows being extracted." << endl;
+    cerr << "  --" << c_row_limit_string << "=N                Optional. Limit the number of rows to N. Otherwise, extract all rows." << endl;
+    cerr << "  --" << c_start_after_string << "=ID             Optional. Assuming a block of rows has already been extracted, start this" << endl;


Question not specifically related to this message (but could have some bearing), what if they specify an id and there has been no previous block? Or they specify an ID that is somewhere in the middle of the previous block or a few rows after the end of the previous block?

That could all be explained, but it's a bit overwhelming. The purpose of the start-after is to obtain multiple blocks instead of all rows. It doesn't matter if they break that rule, as it will work anyway. To start at the beginning, leave the parameter off.

So, really the definition is:
Specifies the ID of the row at which the extract starts.

To extract rows from the database in blocks, use this parameter in conjunction with c_row_limit_string.

The subtlety here is that start-after does not specify the starting ID, it specifies the ID of the row prior to the first one we want. This is because we don't know the ID of the first one - we only know the ID of the previous one.

The algorithm for dumping all rows one block at a time is very simple, but quite a bit to explain in usage(). A developer could figure it out with just a little experimentation.

Specifies the ID of the row that precedes the row at which to start the extract.
Typically this will be the last row of the block that you just extracted.
To extract rows from the database in blocks, use this parameter in conjunction with c_row_limit_string.

vDonGlover

LGTM

improve command-line processing of gaia_db_extract

de2561f

waynelwarren requested review from JackAtGaia and jc-gaiaplatform October 28, 2021 19:22

LaurentiuCristofor reviewed Oct 28, 2021

View reviewed changes

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

daxhaw reviewed Oct 29, 2021

View reviewed changes

addressing PR comments

20a2210

LaurentiuCristofor reviewed Nov 4, 2021

View reviewed changes

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

LaurentiuCristofor approved these changes Nov 4, 2021

View reviewed changes

waynelwarren requested a review from vDonGlover November 4, 2021 04:11

waynelwarren added 2 commits November 4, 2021 08:09

Merge branch 'master' into wayne/improve-extract-main

12fd078

changed c_start_string to c_start_after_string

425d30c

vDonGlover reviewed Nov 4, 2021

View reviewed changes

waynelwarren added 2 commits November 4, 2021 13:07

Merge branch 'master' into wayne/improve-extract-main

48d0125

reworked usage and error messages

8621b90

vDonGlover reviewed Nov 4, 2021

View reviewed changes

waynelwarren added 2 commits November 4, 2021 15:06

reworking the usage text

b054a0c

more usage() improvements

5598c1b

vDonGlover approved these changes Nov 5, 2021

View reviewed changes

waynelwarren merged commit 3af8a0a into master Nov 5, 2021

waynelwarren deleted the wayne/improve-extract-main branch November 5, 2021 18:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve command-line processing of gaia_db_extract #1048

Improve command-line processing of gaia_db_extract #1048

waynelwarren commented Oct 28, 2021

simone-gaia commented Oct 29, 2021 •

edited

Loading

daxhaw Oct 29, 2021

waynelwarren Nov 2, 2021

daxhaw Oct 29, 2021

waynelwarren Nov 2, 2021

daxhaw Oct 29, 2021

waynelwarren Nov 2, 2021

daxhaw Oct 29, 2021

waynelwarren Nov 2, 2021

waynelwarren commented Nov 2, 2021

simone-gaia commented Nov 2, 2021

waynelwarren commented Nov 2, 2021

simone-gaia commented Nov 2, 2021

LaurentiuCristofor left a comment

vDonGlover Nov 4, 2021

vDonGlover Nov 4, 2021

waynelwarren Nov 4, 2021

vDonGlover Nov 4, 2021

vDonGlover Nov 4, 2021

waynelwarren Nov 4, 2021

waynelwarren commented Nov 4, 2021

vDonGlover Nov 4, 2021

waynelwarren Nov 4, 2021

vDonGlover Nov 4, 2021

waynelwarren Nov 4, 2021

vDonGlover Nov 4, 2021

vDonGlover Nov 4, 2021

waynelwarren Nov 4, 2021

vDonGlover Nov 4, 2021

vDonGlover left a comment

Improve command-line processing of gaia_db_extract #1048

Improve command-line processing of gaia_db_extract #1048

Conversation

waynelwarren commented Oct 28, 2021

simone-gaia commented Oct 29, 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

waynelwarren commented Nov 2, 2021

simone-gaia commented Nov 2, 2021

waynelwarren commented Nov 2, 2021

simone-gaia commented Nov 2, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waynelwarren commented Nov 4, 2021

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

vDonGlover left a comment

Choose a reason for hiding this comment

simone-gaia commented Oct 29, 2021 •

edited

Loading