First support for secondary metadata query. #114

aptls · 2017-12-16T01:19:48Z

Add support for a secondary SQL query to retrieve metadata from a
second table. This first implementation supports a model in which
metadata is stored in a second table in one or more rows keyed by
some unique value from the original doc record. Three new config
parameters are used: db.extraMetadataSql (to specify the query),
db.extraMetadataSqlParameters (to list the column names whose
values are used as parameters in db.extraMetadataSql), and
optionally db.extraMetadataColumns (to specify the columns from
the metadata result set to use).

The addMetadata/addMetadataTo* methods were refactored a bit to
allow addMetadata to be used for the extraMetadata table as well.

The current implementation requires the key into the metadata
table to be part of the doc's unique key, but this is not
necessarily a requirement to support retrieving metadata from a
second table.

The config parameters and new methods use "extraMetadata" in
their names to identify them as applying to this secondary
query. The prior existence of "db.metadataColumns" as a parameter
applying to the primary doc query made new parameter names
starting with "db.metadata*" feel confusing. A better naming
scheme might be nice.

This implementation is expected to be extended with support for
other metadata structures, possibly in a manner similar to the
ResponseGenerator, so that should be taken into account when
thinking about config parameter names.

Add support for a secondary SQL query to retrieve metadata from a second table. This first implementation supports a model in which metadata is stored in a second table in one or more rows keyed by some unique value from the original doc record. Three new config parameters are used: db.extraMetadataSql (to specify the query), db.extraMetadataSqlParameters (to list the column names whose values are used as parameters in db.extraMetadataSql), and optionally db.extraMetadataColumns (to specify the columns from the metadata result set to use). The addMetadata/addMetadataTo* methods were refactored a bit to allow addMetadata to be used for the extraMetadata table as well. The current implementation requires the key into the metadata table to be part of the doc's unique key, but this is not necessarily a requirement to support retrieving metadata from a second table. The config parameters and new methods use "extraMetadata" in their names to identify them as applying to this secondary query. The prior existence of "db.metadataColumns" as a parameter applying to the primary doc query made new parameter names starting with "db.metadata*" feel confusing. A better naming scheme might be nice. This implementation is expected to be extended with support for other metadata structures, possibly in a manner similar to the ResponseGenerator, so that should be taken into account when thinking about config parameter names.

googlebot · 2017-12-16T01:19:51Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.
If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot. The email used to register you as an authorized contributor must be the email used for the Git commit.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

wiarlawd

Lots of comments but mostly minor suggestions or questions.

Check the code coverage. It looks like you're missing tests for urlAndMetadataLister, including both getDocIds and getModifiedDocIds.

wiarlawd · 2017-12-19T01:07:52Z

src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java

      }
    }

+    if (!isNullOrEmptyString(cfg.getValue("db.extraMetadataSql"))) {


This can't be null; we've been drawing the conditions more narrowly in new code. I prefer using foo.isEmpty, but "".equals(foo) is popular in the code, too.

wiarlawd · 2017-12-19T01:09:53Z

src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java

+        extraMetadataColumns = new MetadataColumns(extraMetadataColumnsConfig);
+        log.config("extra metadata columns: " + extraMetadataColumns);
+      }
+    }


Add an else to check for non-empty db.extraMetadataColumns and db.extraMetadataSqlParameters and add them to ignored. You should add a test for that as well (I think ignoring both in one test is fine, or maybe even add it to an existing ignored property test; I'm not sure how those are organized.

wiarlawd · 2017-12-19T01:13:35Z

src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java

-  private void addMetadata(MetadataHandler meta, ResultSet rs)
-      throws SQLException, IOException {
+  private void addMetadata(MetadataHandler meta, ResultSet rs,
+      MetadataColumns rsMetadataColumns) throws SQLException, IOException {


Maybe just columns? rsMetadataColumns made me expect a ResultSetMetaData object.

wiarlawd · 2017-12-19T01:25:50Z

src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java

+  private void addExtraMetadataToRecordBuilder(
+      final DocIdPusher.Record.Builder builder, Connection conn,
+      String uniqueId) throws SQLException, IOException {
+


Delete blank line.

wiarlawd · 2017-12-19T01:27:02Z

src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java

+      final DocIdPusher.Record.Builder builder, Connection conn,
+      String uniqueId) throws SQLException, IOException {
+
+    try (PreparedStatement stmt = getExtraMetadataFromDb(conn, uniqueId);


What made you decide against the extra helper method to share this try statement between the two new methods?

wiarlawd · 2017-12-19T01:28:32Z

src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java

    return st;
  }

+  private PreparedStatement getExtraMetadataFromDb(Connection conn,


We're planning to rename these methods, so lets go with prepareExtraMetadataStatement for this one now, so the change doesn't collide with this one.

wiarlawd · 2017-12-19T01:30:06Z

src/com/google/enterprise/adaptor/database/UniqueKey.java

-    return "UniqueKey(" + docIdSqlCols + "," + types + "," + contentSqlCols + ","
-        + aclSqlCols + ")";
+    return "UniqueKey(" + docIdSqlCols + "," + types + ","
+        + contentSqlCols + "," + aclSqlCols + "," + metadataSqlCols + ")";


Why re-wrap this and modify the first line? Random or on purpose?

The line was triggering my 80-cols check and since I was adding to the string anyway, I changed it to get rid of the warning.

wiarlawd · 2017-12-19T01:39:44Z

test/com/google/enterprise/adaptor/database/DatabaseAdaptorTest.java

+
+    Map<String, String> configEntries = new HashMap<String, String>();
+    configEntries.put("db.uniqueKey", "id:int");
+    configEntries.put("db.everyDocIdSql", "select * from data");


Unused, here and the other new tests, so put it after the usual comment to that effect.

wiarlawd · 2017-12-19T01:45:54Z

test/com/google/enterprise/adaptor/database/DatabaseAdaptorTest.java

+    configEntries.put("db.extraMetadataSql",
+        "select * from metadata where id = ?");
+    configEntries.put("db.extraMetadataSqlParameters", "id");
+    configEntries.put("db.extraMetadataColumns", "OTHER");


I think we could either leave this lowercase, or maybe mixed case? Or, since I think we should have a test with a mapping in db.extraMetadataColumns, we could use uppercase in one, as here, and lower case in the other, as "other:col1".

wiarlawd

Mostly ideas for more tests, and the issue with docId.isUrl and db.extraMetadataSql.

wiarlawd · 2017-12-21T01:29:00Z

src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java

+  private void addExtraMetadataToRecordBuilder(
+      final DocIdPusher.Record.Builder builder, Connection conn,
+      String uniqueId) throws SQLException, IOException {
+    MetadataHandler handler = new MetadataHandler() {


Maybe inline this as we do in addMetadataToXxx?

wiarlawd · 2017-12-21T01:33:23Z

src/com/google/enterprise/adaptor/database/UniqueKey.java

  public String toString() {
    return "UniqueKey(" + docIdSqlCols + "," + types + "," + contentSqlCols + ","
-        + aclSqlCols + ")";
+        + aclSqlCols + "," + metadataSqlCols + ")";


Har, OK. Either way is fine.

wiarlawd · 2017-12-21T01:34:02Z

src/com/google/enterprise/adaptor/database/UniqueKey.java


+    Builder setMetadataSqlColumns(String metadataSqlColumns)
+        throws InvalidConfigurationException {
+      if (null == metadataSqlColumns) {


Use Yoda conditions, we do not. (Used in the past, they have been, but no longer.)

wiarlawd · 2017-12-21T01:35:21Z

test/com/google/enterprise/adaptor/database/DatabaseAdaptorTest.java

   }

+  @Test
+  public void testInit_ignoredPropertyes_extraMetadata() throws Exception {


s/Propertyes/Properties/, unless these are specifically ye olde style propertyes.

Also, I think testInitExtraMetadata_..., for consistency with the other names (we aren't using testInit_... anywhere, though perhaps that would be better).

wiarlawd · 2017-12-22T00:30:38Z

src/com/google/enterprise/adaptor/database/UniqueKey.java

-      String columnValue = decodeSlashInData(parts[i]);
-      zip.put(docIdSqlCols.get(i), columnValue);
+    if (docIdIsUrl) {
+      zip.put(docIdSqlCols.get(0), uniqueId);


I don't like this. It feels so special case: if your doc ID is an URL (whether using urlAndMetadataLister mode or not), then your extraMetadataSql can only take that URL as a unique key parameter. We should make that explicit somewhere, maybe in a comment here. As we discussed, I think it would be preferable to try the route you suggested originally where the extraMetadataSqlParameters come from the singleDocContentSql result set, rather than from the doc ID. Maybe just a TODO here for now, I'm OK with exploring that in a separate change, but PJ may not be.

wiarlawd · 2017-12-22T00:35:25Z

src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java

      throws SQLException, IOException {
+    synchronized (this) {
+      if (metadataColumns == null) {
+        metadataColumns = new MetadataColumns(rs.getMetaData());


This line isn't tested, since you lifted this out of the helper in two places, only the other line is executed.

wiarlawd · 2017-12-22T00:42:43Z

src/com/google/enterprise/adaptor/database/DatabaseAdaptor.java

+        addMetadata(handler, rs, extraMetadataColumns);
+      }
+    } catch (SQLException ex) {
+      throw new IOException(ex);


This line is untested. I'm not sure how to trigger this. A flat invalid query would fail in validation. Maybe a type mismatch in the parameter expression: <int-field> = ? with a character parameter? I can look harder if you can't find something easily.

wiarlawd · 2017-12-22T00:47:53Z

test/com/google/enterprise/adaptor/database/DatabaseAdaptorTest.java

+    moreEntries.put("db.uniqueKey", "id:int");
+    moreEntries.put("db.extraMetadataSql",
+        "select other from data where id = ?");
+    moreEntries.put("db.extraMetadataSqlParameters", "id");


Do you have a test that does not specify this? By default, db.uniqueKey should be used, so we should have a test of that, which could maybe be this one. I think we should have some test where the unique key has two columns, one used in singleDocContentSql, and the other used in extraMetadataSql.

wiarlawd · 2017-12-22T00:53:23Z

test/com/google/enterprise/adaptor/database/DatabaseAdaptorTest.java

+  }
+
+  @Test
+  public void testExtraMetadata_extraMetadataColumns_Mapping()


s/Mapping/mapping/

wiarlawd · 2017-12-22T00:58:40Z

test/com/google/enterprise/adaptor/database/DatabaseAdaptorTest.java

        pusher.getRecords());
  }

+  @Test


Another test that I don't see from before that we could have had, is to make sure that getDocIds and getModifiedDocIds really do ignore the metadata config when docIdIsUrl. We have lots of init tests, and one each for getDocIds and getModifiedDocIds, to check the URL and date handling. I think we should either add some metadata to those, or have a new pair of tests with just a single row, and both metadataColumns and extraMetadataSql configured, and verify that no metadata is returned (and maybe extraMetadataSql could be syntactically invalid; though I think we "ignore" the property, but still validate the SQL?).

wiarlawd

Still waiting for PJ's review.

wiarlawd · 2018-01-06T00:35:37Z

src/com/google/enterprise/adaptor/database/UniqueKey.java

  private void setSqlValues(PreparedStatement st, String uniqueId,
      List<String> sqlCols) throws SQLException {
    Map<String, String> zip = new TreeMap<>(String.CASE_INSENSITIVE_ORDER);
+    // Parameters to SQL queries are taken from the unique id. When reading extraMetadata


wiarlawd · 2018-01-06T00:37:19Z

test/com/google/enterprise/adaptor/database/DatabaseAdaptorTest.java

        pusher.getRecords());
  }

+


Delete added blank line.

wiarlawd · 2018-01-06T00:38:22Z

test/com/google/enterprise/adaptor/database/DatabaseAdaptorTest.java

        "select id, name from data where id = ?");
    configEntries.put("db.modeOfOperation", "rowToText");
-    configEntries.put("db.metadataColumns", "name");
+    configEntries.put("db.includeAllColumnsAsMetadata", "true");


Why make this change?

Changing this setting adds coverage for the case you noted above for line 694.

aptls requested review from PapaJoltOmega and wiarlawd December 16, 2017 01:19

wiarlawd suggested changes Dec 19, 2017

View reviewed changes

Code review changes.

567df68

wiarlawd suggested changes Dec 22, 2017

View reviewed changes

Code review changes.

40f13df

wiarlawd approved these changes Jan 6, 2018

View reviewed changes

Code review changes.

6696f19

First support for secondary metadata query. #114

Are you sure you want to change the base?

First support for secondary metadata query. #114

Uh oh!

Conversation

aptls commented Dec 16, 2017

Uh oh!

googlebot commented Dec 16, 2017

Uh oh!

wiarlawd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wiarlawd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wiarlawd Dec 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wiarlawd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

wiarlawd Dec 22, 2017 •

edited

Loading