You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@avro.apache.org by GitBox <gi...@apache.org> on 2022/08/11 12:07:06 UTC
[GitHub] [avro] martin-g opened a new pull request, #1821: AVRO-3601: C++ API header contains breaking include
martin-g opened a new pull request, #1821:
URL: https://github.com/apache/avro/pull/1821
Rename CustomFields to CustomAttributes.
Rework CustomAttributes to keep a map of string name with a string
value. The user application may parse the string to JSON if needed.
Add a new step to the CI workflow to prevent problems like this in the
future
### Jira
- [X] https://issues.apache.org/jira/browse/AVRO-3601
### Tests
- [X] My PR updates the relevant unit tests
### Commits
- [X] My commits all reference Jira issues in their subject lines. In addition, my commits follow the guidelines from "[How to write a good git commit message](https://chris.beams.io/posts/git-commit/)":
1. Subject is separated from body by a blank line
1. Subject is limited to 50 characters (not including Jira issue reference)
1. Subject does not end with a period
1. Subject uses the imperative mood ("add", not "adding")
1. Body wraps at 72 characters
1. Body explains "what" and "why", not "how"
### Documentation
- [ ] In case of new functionality, my PR adds documentation that describes how to use it.
- All the public functions and the classes in the PR contain Javadoc that explain what it does
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] martin-g commented on a diff in pull request #1821: AVRO-3601: C++ API header contains breaking include
Posted by GitBox <gi...@apache.org>.
martin-g commented on code in PR #1821:
URL: https://github.com/apache/avro/pull/1821#discussion_r944181406
##########
lang/c++/impl/CustomAttributes.cc:
##########
@@ -0,0 +1,51 @@
+
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+#include "CustomAttributes.hh"
+#include <map>
+#include <memory>
+#include "Exception.hh"
+
+namespace avro {
+
+std::string CustomAttributes::getAttribute(const std::string &name) const {
+ std::map<std::string, std::string>::const_iterator iter =
+ attributes_.find(name);
+ if (iter == attributes_.end()) {
+ return NULL;
Review Comment:
No, there is no such test.
Suggestions how to rework this ? Maybe return Option(al) ?! I have to see which version of C++ supports this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] martin-g commented on a diff in pull request #1821: AVRO-3601: C++ API header contains breaking include
Posted by GitBox <gi...@apache.org>.
martin-g commented on code in PR #1821:
URL: https://github.com/apache/avro/pull/1821#discussion_r944210050
##########
lang/c++/impl/CustomAttributes.cc:
##########
@@ -0,0 +1,51 @@
+
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+#include "CustomAttributes.hh"
+#include <map>
+#include <memory>
+#include "Exception.hh"
+
+namespace avro {
+
+std::string CustomAttributes::getAttribute(const std::string &name) const {
+ std::map<std::string, std::string>::const_iterator iter =
+ attributes_.find(name);
+ if (iter == attributes_.end()) {
+ return NULL;
Review Comment:
`boost::optional` is available!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] KalleOlaviNiemitalo commented on a diff in pull request #1821: AVRO-3601: C++ API header contains breaking include
Posted by GitBox <gi...@apache.org>.
KalleOlaviNiemitalo commented on code in PR #1821:
URL: https://github.com/apache/avro/pull/1821#discussion_r944334140
##########
lang/c++/impl/Compiler.cc:
##########
@@ -268,14 +268,14 @@ static const std::unordered_set<std::string>& getKnownFields() {
return kKnownFields;
}
-static void getCustomAttributes(const Object& m, CustomFields &customAttributes)
+static void getCustomAttributes(const Object& m, CustomAttributes &customAttributes)
{
// Don't add known fields on primitive type and fixed type into custom
// fields.
const std::unordered_set<std::string>& kKnownFields = getKnownFields();
for (const auto &entry : m) {
if (kKnownFields.find(entry.first) == kKnownFields.end()) {
- customAttributes.addField(entry.first, entry.second);
+ customAttributes.addAttribute(entry.first, entry.second.stringValue());
Review Comment:
I commented further in <https://github.com/apache/avro/pull/1826#discussion_r944321962>.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] martin-g commented on a diff in pull request #1821: AVRO-3601: C++ API header contains breaking include
Posted by GitBox <gi...@apache.org>.
martin-g commented on code in PR #1821:
URL: https://github.com/apache/avro/pull/1821#discussion_r944245733
##########
lang/c++/impl/CustomAttributes.cc:
##########
@@ -0,0 +1,51 @@
+
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+#include "CustomAttributes.hh"
+#include <map>
+#include <memory>
+#include "Exception.hh"
+
+namespace avro {
+
+std::string CustomAttributes::getAttribute(const std::string &name) const {
+ std::map<std::string, std::string>::const_iterator iter =
+ attributes_.find(name);
+ if (iter == attributes_.end()) {
+ return NULL;
Review Comment:
https://github.com/apache/avro/pull/1826
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] martin-g commented on pull request #1821: AVRO-3601: C++ API header contains breaking include
Posted by GitBox <gi...@apache.org>.
martin-g commented on PR #1821:
URL: https://github.com/apache/avro/pull/1821#issuecomment-1274439167
It will be part of 1.11.2 and 1.12.0 when they are released.
There are no discussions about specific dates for now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] martin-g commented on pull request #1821: AVRO-3601: C++ API header contains breaking include
Posted by GitBox <gi...@apache.org>.
martin-g commented on PR #1821:
URL: https://github.com/apache/avro/pull/1821#issuecomment-1211902138
CC @nileyadav @mruffing @chenrui333
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] KalleOlaviNiemitalo commented on a diff in pull request #1821: AVRO-3601: C++ API header contains breaking include
Posted by GitBox <gi...@apache.org>.
KalleOlaviNiemitalo commented on code in PR #1821:
URL: https://github.com/apache/avro/pull/1821#discussion_r943572007
##########
lang/c++/impl/CustomAttributes.cc:
##########
@@ -0,0 +1,51 @@
+
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+#include "CustomAttributes.hh"
+#include <map>
+#include <memory>
+#include "Exception.hh"
+
+namespace avro {
+
+std::string CustomAttributes::getAttribute(const std::string &name) const {
+ std::map<std::string, std::string>::const_iterator iter =
+ attributes_.find(name);
+ if (iter == attributes_.end()) {
+ return NULL;
Review Comment:
Conversion from nullptr to std::string has undefined behavior. Is there a test case that covers this?
<https://github.com/gcc-mirror/gcc/blob/04ce2400b35225302e0d6883bb0817378180f5d7/libstdc%2B%2B-v3/include/bits/basic_string.h#L634-L640>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] thiru-mg merged pull request #1821: AVRO-3601: C++ API header contains breaking include
Posted by GitBox <gi...@apache.org>.
thiru-mg merged PR #1821:
URL: https://github.com/apache/avro/pull/1821
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] KalleOlaviNiemitalo commented on a diff in pull request #1821: AVRO-3601: C++ API header contains breaking include
Posted by GitBox <gi...@apache.org>.
KalleOlaviNiemitalo commented on code in PR #1821:
URL: https://github.com/apache/avro/pull/1821#discussion_r943586881
##########
lang/c++/impl/Compiler.cc:
##########
@@ -268,14 +268,14 @@ static const std::unordered_set<std::string>& getKnownFields() {
return kKnownFields;
}
-static void getCustomAttributes(const Object& m, CustomFields &customAttributes)
+static void getCustomAttributes(const Object& m, CustomAttributes &customAttributes)
{
// Don't add known fields on primitive type and fixed type into custom
// fields.
const std::unordered_set<std::string>& kKnownFields = getKnownFields();
for (const auto &entry : m) {
if (kKnownFields.find(entry.first) == kKnownFields.end()) {
- customAttributes.addField(entry.first, entry.second);
+ customAttributes.addAttribute(entry.first, entry.second.stringValue());
Review Comment:
Are custom attributes required to have string values? I thought the plan was to store a string in JSON syntax, i.e. including quotation marks if the value is a JSON string.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] nileyadav commented on pull request #1821: AVRO-3601: C++ API header contains breaking include
Posted by GitBox <gi...@apache.org>.
nileyadav commented on PR #1821:
URL: https://github.com/apache/avro/pull/1821#issuecomment-1274380039
Thank you @martin-g for promptly fixing this.
Will the CustomAttribute feature and this fix be part of release?
Regards,
Nilesh
On Thu, Aug 11, 2022 at 7:01 AM Thiruvalluvan M G ***@***.***>
wrote:
> Merged #1821 <https://github.com/apache/avro/pull/1821> into master.
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/avro/pull/1821#event-7171940248>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AVR5AVXCG3TTDIMLJNTP26LVYUBSFANCNFSM56H5M4IQ>
> .
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] martin-g commented on a diff in pull request #1821: AVRO-3601: C++ API header contains breaking include
Posted by GitBox <gi...@apache.org>.
martin-g commented on code in PR #1821:
URL: https://github.com/apache/avro/pull/1821#discussion_r944182505
##########
lang/c++/impl/Compiler.cc:
##########
@@ -268,14 +268,14 @@ static const std::unordered_set<std::string>& getKnownFields() {
return kKnownFields;
}
-static void getCustomAttributes(const Object& m, CustomFields &customAttributes)
+static void getCustomAttributes(const Object& m, CustomAttributes &customAttributes)
{
// Don't add known fields on primitive type and fixed type into custom
// fields.
const std::unordered_set<std::string>& kKnownFields = getKnownFields();
for (const auto &entry : m) {
if (kKnownFields.find(entry.first) == kKnownFields.end()) {
- customAttributes.addField(entry.first, entry.second);
+ customAttributes.addAttribute(entry.first, entry.second.stringValue());
Review Comment:
I think the C++ developers here have to step up! :-)
Thanks for the review, @KalleOlaviNiemitalo !
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] martin-g commented on a diff in pull request #1821: AVRO-3601: C++ API header contains breaking include
Posted by GitBox <gi...@apache.org>.
martin-g commented on code in PR #1821:
URL: https://github.com/apache/avro/pull/1821#discussion_r944200068
##########
lang/c++/impl/CustomAttributes.cc:
##########
@@ -0,0 +1,51 @@
+
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+#include "CustomAttributes.hh"
+#include <map>
+#include <memory>
+#include "Exception.hh"
+
+namespace avro {
+
+std::string CustomAttributes::getAttribute(const std::string &name) const {
+ std::map<std::string, std::string>::const_iterator iter =
+ attributes_.find(name);
+ if (iter == attributes_.end()) {
+ return NULL;
Review Comment:
`std::optional` is available since C++ 17. Avro uses 11
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] martin-g commented on a diff in pull request #1821: AVRO-3601: C++ API header contains breaking include
Posted by GitBox <gi...@apache.org>.
martin-g commented on code in PR #1821:
URL: https://github.com/apache/avro/pull/1821#discussion_r944269272
##########
lang/c++/impl/Compiler.cc:
##########
@@ -268,14 +268,14 @@ static const std::unordered_set<std::string>& getKnownFields() {
return kKnownFields;
}
-static void getCustomAttributes(const Object& m, CustomFields &customAttributes)
+static void getCustomAttributes(const Object& m, CustomAttributes &customAttributes)
{
// Don't add known fields on primitive type and fixed type into custom
// fields.
const std::unordered_set<std::string>& kKnownFields = getKnownFields();
for (const auto &entry : m) {
if (kKnownFields.find(entry.first) == kKnownFields.end()) {
- customAttributes.addField(entry.first, entry.second);
+ customAttributes.addAttribute(entry.first, entry.second.stringValue());
Review Comment:
https://github.com/apache/avro/pull/1826/commits/074eef4272bf563a334e42a1027890f0c34dcccc
Looks good ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org