jhump commented on code in PR #3266:
URL: https://github.com/apache/avro/pull/3266#discussion_r1946741083
##########
lang/c++/impl/CustomAttributes.cc:
##########
@@ -16,26 +16,63 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
-#include "CustomAttributes.hh"
-#include "Exception.hh"
+
+#if defined(__clang__)
+// Even though CustomAttributes::ValueMode::STRING is deprecated, we still
have to
+// handle/implement it.
+#pragma clang diagnostic ignored "-Wdeprecated-declarations"
+#endif
+
#include <map>
#include <memory>
+#include "CustomAttributes.hh"
+#include "Exception.hh"
+
+#include "json/JsonDom.hh"
+
namespace avro {
+CustomAttributes::CustomAttributes(ValueMode valueMode) {
+ switch (valueMode) {
+ case ValueMode::STRING:
+ case ValueMode::JSON:
+ valueMode_ = valueMode;
+ break;
+ default:
+ throw Exception("invalid ValueMode: " +
std::to_string(static_cast<int>(valueMode)));
+ }
+}
+
std::optional<std::string> CustomAttributes::getAttribute(const std::string
&name) const {
- std::optional<std::string> result;
- std::map<std::string, std::string>::const_iterator iter =
- attributes_.find(name);
+ auto iter = attributes_.find(name);
if (iter == attributes_.end()) {
- return result;
+ return {};
}
- result = iter->second;
- return result;
+ return iter->second;
}
void CustomAttributes::addAttribute(const std::string &name,
const std::string &value) {
+ // Validate the incoming value.
+ //
+ // NOTE: This is a bit annoying that we accept the data as a string
instead of
+ // as an Entity. That means the compiler must convert the value to a
string only
+ // for this method to convert it back. But we can't directly refer to the
+ // json::Entity type in the signatures for this class (and thus cannot
accept
+ // that type directly as a parameter) because then it would need to be
included
+ // from a header file: CustomAttributes.hh. But the json header files are
not
+ // part of the Avro distribution (intentionally), so CustomAttributes.hh
cannot
+ // #include any of the json header files.
+ const std::string &jsonVal = (valueMode_ == ValueMode::STRING)
+ ? std::move("\"" + value + "\"")
+ : value;
+ try {
Review Comment:
The other approach is to simply remove validation entirely. But I feel bad
doing that seems it makes the API a foot-gun: it becomes easy to accidentally
produce an Avro file that is junk/unreadable, even though the function calls
that produced the file never returned any errors or threw any exceptions.
FWIW, in the _expected_ cases, where custom values are not present in great
number and have values that are simple scalars and short strings, it should be
very cheap relative to the rest of the process of encoding and creating the
Avro file. For the compiler, reading and processing an Avro file, the extra
overhead would be strictly less than the processing needed to initially parse
the JSON schema (likely _much_ less, since custom attributes are typically a
very small portion of the JSON). And I suspect (though have no profile data)
that parsing the JSON schema is already a fairly light-weight part of reading
an Avro file.
@martin-g, thoughts?
--
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-unsubscr...@avro.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
