-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
AVRO-3486: Set namespace with protocol fullName #1637
AVRO-3486: Set namespace with protocol fullName #1637
Conversation
When parsing or creating a protocol, use both the name and namespace given to determine the actual name, namespace and fullName. Reason: the spec says "The name and namespace qualification rules defined for schema objects apply to protocols as well."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good code,
A proposition for code for very specific corner case;
I also suggest to add unit test directly on TestProtocol class (it's more direct than to pass by avro file).
if (lastDot < 0) { | ||
this.name = name; | ||
this.namespace = namespace; | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if namespace is "java" and name is "lang.String"
Here, a suggestion :
StringBuilder namespaceBuilder = new StringBuilder((namespace != null) ? namespace : "");
if (namespaceBuilder.length() > 0 && namespaceBuilder.charAt(namespaceBuilder.length() - 1) != '.') {
namespaceBuilder.append('.');
}
namespaceBuilder.append(name.substring(0, lastDot));
this.namespace = namespaceBuilder.toString();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a protocol has namespace "java" and name "lang.String", then its fullname should be "lang.String" for sure (not "java.lang.String"). But it is not clear from the spec whether types defined in this protocol should then inherit the "java" namespace (as declared) or the "lang" namespace (based on the fullname of the protocol). I don't see any justification for making nested types inherit the "java.lang" namespace.
Relevant parts of the spec:
avro/doc/src/content/xdocs/spec.xml
Lines 296 to 316 in 4e1fefc
<p>In record, enum and fixed definitions, the fullname is | |
determined in one of the following ways:</p> | |
<ul> | |
<li>A name and namespace are both specified. For example, | |
one might use <code>"name": "X", "namespace": | |
"org.foo"</code> to indicate the | |
fullname <code>org.foo.X</code>.</li> | |
<li>A fullname is specified. If the name specified contains | |
a dot, then it is assumed to be a fullname, and any | |
namespace also specified is ignored. For example, | |
use <code>"name": "org.foo.X"</code> to indicate the | |
fullname <code>org.foo.X</code>.</li> | |
<li>A name only is specified, i.e., a name that contains no | |
dots. In this case the namespace is taken from the most | |
tightly enclosing schema or protocol. For example, | |
if <code>"name": "X"</code> is specified, and this occurs | |
within a field of the record definition | |
of <code>org.foo.Y</code>, then the fullname | |
is <code>org.foo.X</code>. If there is no enclosing | |
namespace then the null namespace is used.</li> | |
</ul> |
avro/doc/src/content/xdocs/spec.xml
Lines 850 to 851 in 4e1fefc
<p>The name and namespace qualification rules defined for schema objects | |
apply to protocols as well.</p> |
In particular:
For example, if
"name": "X"
is specified, and this occurs within a field of the record definition oforg.foo.Y
, then the fullname isorg.foo.X
.
If that is intended to apply only when there is a "namespace":"org.foo"
property in the record definition or in some outer definition, and not when the record definition has "name":"org.foo.Y", "namespace":"something_else"
, then the specification should say so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The intent is to use the same namespace the the definition itself, and any enclosed definitions that don't specify one themselves. This is a lot easier to comprehend than using different namespaces.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Adds an explicit test as suggested in the PR. This adds a test case that was missing in the `.avpr` tests.
…486-protocol-namespace-parsing
* AVRO-3486: Set namespace with protocol fullName When parsing or creating a protocol, use both the name and namespace given to determine the actual name, namespace and fullName. Reason: the spec says "The name and namespace qualification rules defined for schema objects apply to protocols as well."
* AVRO-3486: Set namespace with protocol fullName When parsing or creating a protocol, use both the name and namespace given to determine the actual name, namespace and fullName. Reason: the spec says "The name and namespace qualification rules defined for schema objects apply to protocols as well."
When parsing or creating a protocol, use both the name and namespace
given to determine the actual name, namespace and fullName.
Reason: the spec says "The name and namespace qualification rules
defined for schema objects apply to protocols as well."
Make sure you have checked all steps below.
Jira
Tests
addsupdates the following unit tests OR does not need testing for this extremely good reason:org.apache.avro.compiler.idl.TestIdl#runTests()
forimport.avdl
andnestedimport.avdl
by updatingbar.avpr
Commits
Documentation