Responses to David's pre-CR review

Hi David,

I worked through your feedback and addressed most of it as proposed.

Diff of the changes:
http://www.w3.org/2001/sw/rdb2rdf/r2rml/diffs/1.187-1.188.html

Complete diff of all changes since LC:
http://www.w3.org/2001/sw/rdb2rdf/r2rml/diffs/1.160-1.188.html

Point-by-point response follows.

David McNeil wrote:
> Section 2.6
> 
> * "A final example" - with the addition of section 2.7 this is no longer the "final" example.

Fixed.

> * I think this example would be more clear if the EMP.DEPTNO column is removed. This column is not referenced by the example and is redundant considering the EMP2DEPT

That's reasonable. Column removed.

> Section 4
> 
> * "Both algorithms are euqivalent..." - "equivalent" is mis-spelled

Fixed.

> Section 4.3
> 
> * My last call comment on this still stands: Doesn't this definition of a mandatory error contradict our stated intent of: if data values fall outside of the intersection of SQL data types and XSD data types then the behavior is undefined (but throwing an error is not required)? I don't remember seeing a place in the spec that captures our intent regarding the intersection of SQL and XSD data types.

I believe this was addressed to your satisfaction in the other thread. I did not change the text.

> Section 7.3
> 
> * "in the data values of either column" - Would it be helpful to add the word "referenced", so this would read: "in the data values of either _referenced_ column"?

Changed. I also replaced “contains multiple columns” with “references multiple columns” earlier in the sentence for consistency.

> Section 7.6
> 
> * "Typable term maps may generate..." - I think the word "Typable" should be replaced with "Datatypable" to be consistent with the rest of this section.

Fixed.

> Section 7.7
> 
> * "... for mapping the DEPT table in this example mapping, an inverse expression..." - I think this points to the wrong example. I think the intent is to point to the example at the end of section 5.2 (rather than section 2.4).

Good catch. I fixed the link.

The example also used two SQL functions that are not standard SQL: substr and length. I replaced them as required by the letter of the spec: SUBSTRING and CHARACTER_LENGTH.

> Section 8
> 
> * "SELECT * FROM ({child-query}) AS tmp" - I would change this to just: child-query or: {child-query}

I think there was a reason for this, but I can't remember it right now. I'd need to think about it. The current version is perhaps unnecessarily verbose but that seems harmless.

> Section 10.2
> 
> * "...would be in error.." - Is this intended to say "would be _an_ error"?

I thought “to be in error” was ok English? Anyway, I changed it “would be an error”.

> Section 11.2
> 
> * "Return a blank node whose blank node identifier is the natural RDF lexical form corresponding to value." - In the face of multiple output graphs, this algorithm seems too simplistic. My concern is that an implementer literally follows this guideline and produces results where blank node identifiers collide between graphs.

I added a link to Section 9.1 after that sentence, and did the same at the end of 11.1 where placing triples into graphs is discussed. Section 9.1 is explicit: “If, however, the same blank node identifier occurs in multiple graphs, then a distinct blank node is created for each graph.”

> Section B1
> 
> * The classname of "BaseTableOrView" stands out as particularly cumbersome. I would change this to "SQLTable". I would let the "or view" part be defined in the formal definition but understood in the type name. I would also change R2RMLView to SQLQuery. This would lead to a tight, clear set of type names: a LogicalTable could be either a SQLTable or SQLQuery.

I sympathise, but authors never need to spell out the class names (they are implicit), so I can live with the slightly cumbersome names.

All the best,
Richard

Received on Tuesday, 24 January 2012 18:02:41 UTC