Re: [PR] [AVRO-3710] [C++] Permit DataFile use without stream ownership [avro]

[via GitHub]

KalleOlaviNiemitalo commented on code in PR #3056:
URL: https://github.com/apache/avro/pull/3056#discussion_r1701594614


##
lang/c++/impl/DataFile.cc:
##
@@ -63,35 +64,34 @@ boost::iostreams::zlib_params get_zlib_params() {
 }
 } // namespace
 
-DataFileWriterBase::DataFileWriterBase(const char *filename, const ValidSchema 
, size_t syncInterval,
-   Codec codec) : filename_(filename),
-  schema_(schema),
+DataFileWriterBase::DataFileWriterBase(const char *filename,
+   const ValidSchema ,
+   size_t syncInterval,
+   Codec codec) : 
DataFileWriterBase(fileOutputStream(filename), schema, syncInterval, codec) {
+}
+
+DataFileWriterBase::DataFileWriterBase(OutputStream ,

Review Comment:
   That seems possible, if the DataFileWriterBase is heap-allocated.  But 
[std::back_insert_iterator](https://en.cppreference.com/w/cpp/iterator/back_insert_iterator/back_insert_iterator)
 likewise takes and saves a reference so programmers should know about this 
kind of risk.



-- 
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

Re: [PR] [AVRO-3710] [C++] Permit DataFile use without stream ownership [avro]

[via GitHub]

mkmkme commented on code in PR #3056:
URL: https://github.com/apache/avro/pull/3056#discussion_r1701535646


##
lang/c++/impl/DataFile.cc:
##
@@ -63,35 +64,34 @@ boost::iostreams::zlib_params get_zlib_params() {
 }
 } // namespace
 
-DataFileWriterBase::DataFileWriterBase(const char *filename, const ValidSchema 
, size_t syncInterval,
-   Codec codec) : filename_(filename),
-  schema_(schema),
+DataFileWriterBase::DataFileWriterBase(const char *filename,
+   const ValidSchema ,
+   size_t syncInterval,
+   Codec codec) : 
DataFileWriterBase(fileOutputStream(filename), schema, syncInterval, codec) {
+}
+
+DataFileWriterBase::DataFileWriterBase(OutputStream ,

Review Comment:
   I might be missing something here, but can't this constructor make 
`DataFileWriterBase` outlive `OutputStream` that it uses?



-- 
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

Re: [PR] AVRO-2843: [PHP] Copy composer setup from Apache Thrift [avro]

[via GitHub]

jjatria commented on code in PR #3057:
URL: https://github.com/apache/avro/pull/3057#discussion_r1700426481


##
composer.json:
##
@@ -3,16 +3,65 @@
   "description": "Apache Avro™ is a data serialization system.",
   "minimum-stability": "stable",
   "license": "Apache-2.0",
+  "homepage": "http://avro.apache.org;,
+  "type": "library",
+  "keywords": [
+"RPC"

Review Comment:
   Is this the correct keyword to use for avro?



-- 
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

Re: [PR] AVRO-2843: [PHP] Copy composer setup from Apache Thrift [avro]

[via GitHub]

jjatria commented on PR #3057:
URL: https://github.com/apache/avro/pull/3057#issuecomment-2263274584

   The risky warnings come from 
https://github.com/apache/avro/blob/main/lang/php/phpunit.xml#L22, which we 
could either set to false for now, or we could add the annotations it expects. 
Either way, this seems to be ready :)


-- 
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

Re: [PR] AVRO-2843: [PHP] Copy composer setup from Apache Thrift [avro]

[via GitHub]

jjatria commented on PR #3057:
URL: https://github.com/apache/avro/pull/3057#issuecomment-2263232307

   I made one additional change to `lang/php/test/test_helper.php` so it (like 
`build.sh`) points to the `vendor` directory at the repo root level rather than 
one inside `lang/php`. Locally, that allows the tests to pass, although I get 
this as a summary:
   
   > OK, but incomplete, skipped, or risky tests!
   > Tests: 415, Assertions: 969, Risky: 415.
   
   Most of the "risky" tests complain like this:
   ```
   This test does not have a @covers annotation but is expected to have one
   ```
   but that sees to me like it should be addressed on a separate PR to keep the 
scope limited.


-- 
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

Re: [PR] AVRO-2843: [PHP] Copy composer setup from Apache Thrift [avro]

[via GitHub]

adamquaile commented on code in PR #3057:
URL: https://github.com/apache/avro/pull/3057#discussion_r1700166474


##
composer.json:
##
@@ -3,16 +3,65 @@
   "description": "Apache Avro™ is a data serialization system.",
   "minimum-stability": "stable",
   "license": "Apache-2.0",
+  "homepage": "http://avro.apache.org;,
+  "type": "library",
+  "keywords": [
+"RPC"
+  ],
+  "readme": "README.md",
+  "authors": [
+{
+  "name": "Apache Avro Developers",
+  "email": "d...@avro.apache.org",
+  "homepage": "http://avro.apache.org;
+}
+  ],
+  "support": {
+"email": "d...@avro.apache.org",
+"issues": "https://issues.apache.org/jira/browse/AVRO;
+  },
   "require": {
-"beberlei/composer-monorepo-plugin": "0.16.5"

Review Comment:
   I also currently hit install issues with the old version. I'm testing with a 
brand new empty project, with the following `composer.json`:
   
   ```json
   {
   "require": {
   "apache/avro": "dev-main"
   },
   "repositories": [
   {
   "type": "vcs",
   "url": "https://github.com/apache/avro;
   }
   ],
   "config": {
   "allow-plugins": {
   "beberlei/composer-monorepo-plugin": true
   }
   }
   }
   ```
   
   ```bash
   composer install
   No composer.lock file present. Updating dependencies to latest instead of 
installing from lock file. See https://getcomposer.org/install for more 
information.
   Loading composer repositories with package information
   Updating dependencies
   Lock file operations: 2 installs, 0 updates, 0 removals
 - Locking apache/avro (dev-main 35b01df)
 - Locking beberlei/composer-monorepo-plugin (v0.16.5)
   Writing lock file
   Installing dependencies from lock file (including require-dev)
   Package operations: 1 install, 1 update, 0 removals
 - Installing beberlei/composer-monorepo-plugin (v0.16.5): Extracting 
archive
 - Upgrading apache/avro (dev-AVRO-2843-packagist 4c1ec59 => dev-main 
35b01df): Extracting archive
   Generating autoload files
   Generating autoload files for monorepo sub-packages with dev-dependencies.
   PHP Fatal error:  Declaration of 
Monorepo\Composer\EventDispatcher::dispatch($eventName, 
?Composer\EventDispatcher\Event $event = null) must be compatible with 
Composer\EventDispatcher\EventDispatcher::dispatch(?string $eventName, 
?Composer\EventDispatcher\Event $event = null): int in 
/Users/aquaile/Development/avro-tests/vendor/beberlei/composer-monorepo-plugin/src/main/Monorepo/Composer/EventDispatcher.php
 on line 9
   
   Fatal error: Declaration of 
Monorepo\Composer\EventDispatcher::dispatch($eventName, 
?Composer\EventDispatcher\Event $event = null) must be compatible with 
Composer\EventDispatcher\EventDispatcher::dispatch(?string $eventName, 
?Composer\EventDispatcher\Event $event = null): int in 
/Users/aquaile/Development/avro-tests/vendor/beberlei/composer-monorepo-plugin/src/main/Monorepo/Composer/EventDispatcher.php
 on line 9
   ```
   
   Pointing to the new branch with this works:
   
   ```json
   {
   "require": {
   "apache/avro": "dev-AVRO-2843-packagist"
   },
   "repositories": [
   {
   "type": "vcs",
   "url": "https://github.com/cv-library/avro;
   }
   ]
   }
   
   ```



-- 
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

Re: [PR] AVRO-2843: [PHP] Copy composer setup from Apache Thrift [avro]

[via GitHub]

adamquaile commented on code in PR #3057:
URL: https://github.com/apache/avro/pull/3057#discussion_r1700044839


##
composer.json:
##
@@ -3,16 +3,65 @@
   "description": "Apache Avro™ is a data serialization system.",
   "minimum-stability": "stable",
   "license": "Apache-2.0",
+  "homepage": "http://avro.apache.org;,
+  "type": "library",
+  "keywords": [
+"RPC"
+  ],
+  "readme": "README.md",
+  "authors": [
+{
+  "name": "Apache Avro Developers",
+  "email": "d...@avro.apache.org",
+  "homepage": "http://avro.apache.org;
+}
+  ],
+  "support": {
+"email": "d...@avro.apache.org",
+"issues": "https://issues.apache.org/jira/browse/AVRO;
+  },
   "require": {
-"beberlei/composer-monorepo-plugin": "0.16.5"

Review Comment:
   My understanding here is that the monorepo plugin is designed to allow 
publishing multiple php packages from the same repository. The important part 
it's currently doing is using the 
[monorepo.json](https://github.com/apache/avro/blob/35b01df443445e5a9193b335b0416075ec469b3a/lang/php/monorepo.json)
 file as a discovery-mechanism of sorts to locate the php source files. 
   
   Unless I'm missing something the `autoload.psr-4` section of this new 
version will serve that need instead via [PSR-4 
autoloading](https://getcomposer.org/doc/04-schema.md#psr-4) and the 
`archive.exclude` with the exclamation mark ensures only the php parts of this 
repository are included.
   
   ~I think we could even delete the `monorepo.json` file too.~ Edit: I can see 
it's already deleted



-- 
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

Re: [PR] AVRO-2843: [PHP] Copy composer setup from Apache Thrift [avro]

[via GitHub]

adamquaile commented on code in PR #3057:
URL: https://github.com/apache/avro/pull/3057#discussion_r1700044839


##
composer.json:
##
@@ -3,16 +3,65 @@
   "description": "Apache Avro™ is a data serialization system.",
   "minimum-stability": "stable",
   "license": "Apache-2.0",
+  "homepage": "http://avro.apache.org;,
+  "type": "library",
+  "keywords": [
+"RPC"
+  ],
+  "readme": "README.md",
+  "authors": [
+{
+  "name": "Apache Avro Developers",
+  "email": "d...@avro.apache.org",
+  "homepage": "http://avro.apache.org;
+}
+  ],
+  "support": {
+"email": "d...@avro.apache.org",
+"issues": "https://issues.apache.org/jira/browse/AVRO;
+  },
   "require": {
-"beberlei/composer-monorepo-plugin": "0.16.5"

Review Comment:
   My understanding here is that the monorepo plugin is designed to allow 
publishing multiple php packages from the same repository. The important part 
it's currently doing is using the 
[monorepo.json](https://github.com/apache/avro/blob/35b01df443445e5a9193b335b0416075ec469b3a/lang/php/monorepo.json)
 file as a discovery-mechanism of sorts to locate the php source files. 
   
   Unless I'm missing something the `autoload.psr-4` section of this new 
version will serve that need instead via [PSR-4 
autoloading](https://getcomposer.org/doc/04-schema.md#psr-4) and the 
`archive.exclude` with the exclamation mark ensures only the php parts of this 
repository are included.
   
   I think we could even delete the `monorepo.json` file too. 



-- 
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

Re: [PR] AVRO-2843: [PHP] Copy composer setup from Apache Thrift [avro]

[via GitHub]

jjatria commented on code in PR #3057:
URL: https://github.com/apache/avro/pull/3057#discussion_r168169


##
composer.json:
##
@@ -3,16 +3,65 @@
   "description": "Apache Avro™ is a data serialization system.",
   "minimum-stability": "stable",
   "license": "Apache-2.0",
+  "homepage": "http://avro.apache.org;,
+  "type": "library",
+  "keywords": [
+"RPC"
+  ],
+  "readme": "README.md",
+  "authors": [
+{
+  "name": "Apache Avro Developers",
+  "email": "d...@avro.apache.org",
+  "homepage": "http://avro.apache.org;
+}
+  ],
+  "support": {
+"email": "d...@avro.apache.org",
+"issues": "https://issues.apache.org/jira/browse/AVRO;
+  },
   "require": {
-"beberlei/composer-monorepo-plugin": "0.16.5"

Review Comment:
   My packagist-fu is almost non-existent, but Thrift also uses a 
monorepo-based setup, and in their case they don't seem to need that plugin. 
I'm more than happy to re-instate the plugin if it is actually needed. I'll try 
to take a look at those failing tests



-- 
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

Re: [PR] AVRO-2843: [PHP] Copy composer setup from Apache Thrift [avro]

[via GitHub]

martin-g commented on code in PR #3057:
URL: https://github.com/apache/avro/pull/3057#discussion_r165070


##
composer.json:
##
@@ -3,16 +3,65 @@
   "description": "Apache Avro™ is a data serialization system.",
   "minimum-stability": "stable",
   "license": "Apache-2.0",
+  "homepage": "http://avro.apache.org;,
+  "type": "library",
+  "keywords": [
+"RPC"
+  ],
+  "readme": "README.md",
+  "authors": [
+{
+  "name": "Apache Avro Developers",
+  "email": "d...@avro.apache.org",
+  "homepage": "http://avro.apache.org;
+}
+  ],
+  "support": {
+"email": "d...@avro.apache.org",
+"issues": "https://issues.apache.org/jira/browse/AVRO;
+  },
   "require": {
-"beberlei/composer-monorepo-plugin": "0.16.5"

Review Comment:
   
https://issues.apache.org/jira/browse/AVRO-2752?focusedCommentId=17870165=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-17870165
   According to Siad this plugin does something important here! But AFAIU the 
idea is to use `archive > exclude` below instead!



-- 
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

Re: [PR] [No Jira] [Rust] fix links to spec [avro]

[via GitHub]

xxchan commented on PR #3052:
URL: https://github.com/apache/avro/pull/3052#issuecomment-2262493158

   Thanks again for you all. ❤️ 
   
   Personally I don't mind too much whether this can be cherry-picked into 
0.17. (It's definitely good to also have this if we have other cherry-picks 
though.)
   
   And no mean to put pressure, but just think "merge earlier" can save our 
energy 


-- 
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

Re: [PR] [No Jira] [Rust] fix links to spec [avro]

[via GitHub]

Fokko commented on code in PR #3052:
URL: https://github.com/apache/avro/pull/3052#discussion_r1699659006


##
lang/rust/avro/src/schema.rs:
##
@@ -1022,7 +1022,7 @@ impl Schema {
 /// Converts `self` into its [Parsing Canonical Form].
 ///
 /// [Parsing Canonical Form]:
-/// 
https://avro.apache.org/docs/1.8.2/spec.html#Parsing+Canonical+Form+for+Schemas
+/// 
https://avro.apache.org/docs/1.8.2/specification/#parsing-canonical-form-for-schemas

Review Comment:
   ```suggestion
   /// 
https://avro.apache.org/docs/current/specification/#parsing-canonical-form-for-schemas
   ```



##
lang/rust/avro/src/schema.rs:
##
@@ -1032,9 +1032,9 @@ impl Schema {
 /// Generate [fingerprint] of Schema's [Parsing Canonical Form].
 ///
 /// [Parsing Canonical Form]:
-/// 
https://avro.apache.org/docs/1.8.2/spec.html#Parsing+Canonical+Form+for+Schemas
+/// 
https://avro.apache.org/docs/1.8.2/specification/#parsing-canonical-form-for-schemas

Review Comment:
   ```suggestion
   /// 
https://avro.apache.org/docs/current/specification/#parsing-canonical-form-for-schemas
   ```



##
lang/rust/avro/src/schema.rs:
##
@@ -2143,7 +2143,7 @@ impl Serialize for RecordField {
 }
 
 /// Parses a **valid** avro schema into the Parsing Canonical Form.
-/// 
https://avro.apache.org/docs/1.8.2/spec.html#Parsing+Canonical+Form+for+Schemas
+/// 
https://avro.apache.org/docs/1.8.2/specification/#parsing-canonical-form-for-schemas

Review Comment:
   ```suggestion
   /// 
https://avro.apache.org/docs/current/specification/#parsing-canonical-form-for-schemas
   ```



-- 
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

Re: [PR] [No Jira] [Rust] fix links to spec [avro]

[via GitHub]

martin-g commented on PR #3052:
URL: https://github.com/apache/avro/pull/3052#issuecomment-2262357361

   We can't have it in 0.17.0 unless the vote is cancelled for some serious 
reason. The broken links in rustdoc is not a reason to cancel the vote!
   Merging it now or after few days does not make much difference. The only 
question is to which other branches to backport it too. This is not very clear 
at the moment!
   
   > Or do you just mean you are busy with releasing, and want to handle PRs 
only after the release is done?
   
   I am busy with my daily job and my personal life! 
   Avro is a hobby for me! I work on it when I want to do it!
   
   
   > It could become outdated, conflicted, and inactive
   
   The past does not show any evidence of such kind of problems (in the Rust 
SDK)!


-- 
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

Re: [PR] [No Jira] [Rust] fix links to spec [avro]

[via GitHub]

xxchan commented on PR #3052:
URL: https://github.com/apache/avro/pull/3052#issuecomment-2262303456

   Hi @martin-g, thanks for the reply, but I'm a little confused: Do you mean 
we want or not want this into 0.17.0? I think actually either option wouldn't 
prevent us from merging this trivial PR. 藍
   
   Or do you just mean you are busy with releasing, and want to handle PRs only 
after the release is done?


-- 
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

Re: [PR] [No Jira] [Rust] fix links to spec [avro]

[via GitHub]

Xuanwo commented on PR #3052:
URL: https://github.com/apache/avro/pull/3052#issuecomment-2262247147

   > I will handle the PR once the 1.12.0/0.17.0 release is done! The tag is 
already cut, so merging it now won't change anything for the rustdocs of 
0.17.0...
   
   Hi @martin-g, thanks a lot for maintaining this. I'm a bit surprised to see 
the PR left open as it is. It could become outdated, conflicted, and inactive. 
Are there any other concerns preventing us from merging?


-- 
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

Re: [PR] [No Jira] [Rust] fix links to spec [avro]

[via GitHub]

martin-g commented on PR #3052:
URL: https://github.com/apache/avro/pull/3052#issuecomment-2262232333

   I will handle the PR once the 1.12.0/0.17.0 release is done!
   The tag is already cut, so merging it now won't change anything for the 
rustdocs of 0.17.0...


-- 
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

Re: [PR] Bump env_logger from 0.10.2 to 0.11.3 in /lang/rust [avro]

[via GitHub]

martin-g commented on PR #2788:
URL: https://github.com/apache/avro/pull/2788#issuecomment-2262223759

   > Hi, @martin-g, I believe this PR can be closed now.
   
   I didn't merge this PR yet because it was requiring newer Rust than 1.70.0. 
`env_logger` is used only as a dev-dependency and there was no real need to 
bump MSRV because of it.
   I will handle it once 1.12.0/0.17.0 is released!


-- 
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

Re: [PR] Bump env_logger from 0.10.2 to 0.11.3 in /lang/rust [avro]

[via GitHub]

martin-g commented on PR #2788:
URL: https://github.com/apache/avro/pull/2788#issuecomment-2262220717

   @dependabot rebase


-- 
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

Re: [PR] Bump env_logger from 0.10.2 to 0.11.3 in /lang/rust [avro]

[via GitHub]

dependabot[bot] commented on PR #2788:
URL: https://github.com/apache/avro/pull/2788#issuecomment-2262220785

   Looks like this PR has been edited by someone other than Dependabot. That 
means Dependabot can't rebase it - sorry!
   
   If you're happy for Dependabot to recreate it from scratch, overwriting any 
edits, you can request `@dependabot recreate`.
   


-- 
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

Re: [PR] WIP: AVRO-3986: [csharp] - Plain JSON encoding for Apache Avro [avro]

[via GitHub]

martin-g commented on PR #2888:
URL: https://github.com/apache/avro/pull/2888#issuecomment-2262217526

   > Hi, I believe this PR has been marked as `rust` in wrong.
   
   The PR diff shows modified files in C#, Java and Rust. 
   There are 26 commits by several authors (me included) - that makes me think 
the author needs to rebase to `main`, or create a fresh new branch/PR and 
cherry-pick only his commits!


-- 
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

Re: [PR] WIP: AVRO-3986: [csharp] - Plain JSON encoding for Apache Avro [avro]

[via GitHub]

Xuanwo commented on PR #2888:
URL: https://github.com/apache/avro/pull/2888#issuecomment-2260979417

   Hi, I believe this PR has been marked as `rust` in wrong.


-- 
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

Re: [PR] Bump env_logger from 0.10.2 to 0.11.3 in /lang/rust [avro]

[via GitHub]

Xuanwo commented on PR #2788:
URL: https://github.com/apache/avro/pull/2788#issuecomment-2260978408

   Hi, @martin-g, I believe this PR can be closed 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-unsubscr...@avro.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] [No Jira] [Rust] fix links to spec [avro]

[via GitHub]

Xuanwo commented on PR #3052:
URL: https://github.com/apache/avro/pull/3052#issuecomment-2260935099

   Hi @fokko, This is just a minor document update. Could you take a look and 
review it? Thanks!


-- 
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

Re: [PR] [NO JIRA] [C++] Add local-timestamp-millis, local-timestamp-micros logical types [avro]

[via GitHub]

glywk commented on PR #3053:
URL: https://github.com/apache/avro/pull/3053#issuecomment-2258924339

   > I worried a bit about potential bugs in conversions between Avro 
timestamps and `std::chrono::` types; but AFAICT this library doesn't implement 
any such conversions, so it can't have any bugs in them.
   
   You are right `std::chrono::milliseconds` and `std::chrono::microseconds` 
can be less large 
[chrono::duration](https://en.cppreference.com/w/cpp/chrono/duration) than 
`avro long` (int64_t). But it is not the aim of this pull request.


-- 
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

Re: [PR] [NO JIRA] [C++] Add local-timestamp-millis, local-timestamp-micros logical types [avro]

[via GitHub]

KalleOlaviNiemitalo commented on code in PR #3053:
URL: https://github.com/apache/avro/pull/3053#discussion_r1696803127


##
lang/c++/impl/Node.cc:
##
@@ -189,6 +190,18 @@ void Node::setLogicalType(LogicalType logicalType) {
 "LONG type");
 }
 break;
+case LogicalType::LOCAL_TIMESTAMP_MILLIS:
+if (type_ != AVRO_LONG) {
+throw Exception("LOCAL_TIMESTAMP-MILLIS logical type can only 
annotate "
+"LONG type");
+}
+break;
+case LogicalType::LOCAL_TIMESTAMP_MICROS:
+if (type_ != AVRO_LONG) {
+throw Exception("LOCAL_TIMESTAMP-MICROS logical type can only 
annotate "
+"LONG type");

Review Comment:
   ```suggestion
   throw Exception("LOCAL-TIMESTAMP-MICROS logical type can 
only annotate "
   "LONG type");
   ```



##
lang/c++/impl/Node.cc:
##
@@ -189,6 +190,18 @@ void Node::setLogicalType(LogicalType logicalType) {
 "LONG type");
 }
 break;
+case LogicalType::LOCAL_TIMESTAMP_MILLIS:
+if (type_ != AVRO_LONG) {
+throw Exception("LOCAL_TIMESTAMP-MILLIS logical type can only 
annotate "
+"LONG type");

Review Comment:
   ```suggestion
   throw Exception("LOCAL-TIMESTAMP-MILLIS logical type can 
only annotate "
   "LONG type");
   ```



-- 
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

Re: [PR] AVRO-4024: [Rust] support nan/inf/-inf as float/double default [avro]

[via GitHub]

xxchan commented on PR #3051:
URL: https://github.com/apache/avro/pull/3051#issuecomment-2254908712

   @KalleOlaviNiemitalo I've tested with the java lib. And `"NaN"` is allowed, 
but not `NaN`
   
   ```java
   final String schemaString = "{\"type\" : \"record\", \"name\" : \"R\",\n"
   + "  \"fields\" : [ \n   {\"name\" : \"f\", \"type\" : \"float\", 
\"default\": \"NaN\"}]}";
   final Schema schema = new Schema.Parser().parse(schemaString);
   ```
   
   Note that this is handled inside jackason:
   - `"NaN"` is parsed into a `DoubleNode`, and `isNumber` returns `true` in 
`isValidDefault` you linked.
   - On the contrary, `NaN` will returns exception:
   ```
   org.apache.avro.SchemaParseException: 
com.fasterxml.jackson.core.JsonParseException: Non-standard token 'NaN': enable 
`JsonReadFeature.ALLOW_NON_NUMERIC_NUMBERS` to allow
   ```
   
   
   Anyway, thanks for pointing out that Avro IDL is a different thing. I should 
have tested with the java lib directly at the beginning :)


-- 
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

Re: [PR] AVRO-4000 Upgrade parent pom to 33 [avro]

[via GitHub]

slachiewicz commented on PR #2955:
URL: https://github.com/apache/avro/pull/2955#issuecomment-2254463690

   I've updated to use our recently released Parent POM 33 and rebased. 
   


-- 
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

Re: [PR] AVRO-4024: [Rust] support nan/inf/-inf as float/double default [avro]

[via GitHub]

xxchan commented on PR #3051:
URL: https://github.com/apache/avro/pull/3051#issuecomment-2254428588

   Thanks for these information! This problem seems more complex than I 
expected. Let me do more research on it.


-- 
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

Re: [PR] AVRO-4024: [Rust] support nan/inf/-inf as float/double default [avro]

[via GitHub]

KalleOlaviNiemitalo commented on PR #3051:
URL: https://github.com/apache/avro/pull/3051#issuecomment-2254332175

   More specific link: 



-- 
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

Re: [PR] AVRO-4024: [Rust] support nan/inf/-inf as float/double default [avro]

[via GitHub]

KalleOlaviNiemitalo commented on PR #3051:
URL: https://github.com/apache/avro/pull/3051#issuecomment-2254331725

   The code you linked to is in the Java implementation of Avro IDL.  The Java 
code that parses Avro schemas from JSON does not allow `"default": "NaN"` for a 
floating-point field: 

   
   For Avro IDL, the Java implementation seems to expect `float val = NaN;` 
without quotation marks, rather than `float val = "NaN";`.  Also, it allows 
hexadecimal floating-point literals, hexadecimal integer literals, and octal 
integer literals, even though those are not valid in JSON.
   
   So, this part of the Avro IDL specification at 

   
   > Default values for fields may be optionally specified by using an equals 
sign after the field name followed by a JSON expression indicating the default 
value. This JSON is interpreted as described in the 
[spec](https://avro.apache.org/docs/1.11.1/specification/#schema-record).
   
   fails to describe the de-facto support for those non-JSON syntaxes.
   
   Now if you change the Avro specification to allow `"NaN"`, `"Infinity"`, and 
`"-Infinity"` as default values in JSON, I think the Avro IDL specification 
must also be changed to state that those quoted forms are not valid in Avro IDL 
and that the unquoted literals must be used instead.
   


-- 
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

Re: [PR] [docs] Remove 'default' from map and array examples (and add to enum). [avro]

[via GitHub]

HuwCampbell commented on PR #3046:
URL: https://github.com/apache/avro/pull/3046#issuecomment-2254140507

   This is in 
[Schema.java](https://github.com/apache/avro/blob/main/lang/java/avro/src/main/java/org/apache/avro/Schema.java#L1235C1-L1245C4).
   
   ```java
   @Override
   @Deprecated
   void toJson(Set knownNames, String currentNamespace, JsonGenerator 
gen) throws IOException {
 gen.writeStartObject();
 gen.writeStringField("type", "map");
 gen.writeFieldName("values");
 valueType.toJson(knownNames, currentNamespace, gen);
 writeProps(gen);
 gen.writeEndObject();
   }
   ```
   
   Props are written, but there's no default value to be seen.
   


-- 
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

Re: [PR] [docs] Remove 'default' from map and array examples (and add to enum). [avro]

[via GitHub]

xxchan commented on PR #3046:
URL: https://github.com/apache/avro/pull/3046#issuecomment-2254131539

   BTW, I don't think Rust implementation is stable enough as a reference. Can 
you link to related Java code?


-- 
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

Re: [PR] AVRO-4024: [Rust] support nan/inf/-inf as float/double default [avro]

[via GitHub]

xxchan commented on PR #3051:
URL: https://github.com/apache/avro/pull/3051#issuecomment-2254099496

   1. This behavior has existed in java (the de-facto standard) for >10 years, 
and in the real world such schema do exists. So I don't think there are any 
practical reason why the Rust implementation should reject it. 
https://github.com/apache/avro/blob/42c54e68120bb5c2393954b5990a0aa3b3960510/lang/java/compiler/src/main/javacc/org/apache/avro/compiler/idl/idl.jj#L318-L322
   2. Perhaps we can "guess the intention" of the spec: First, float/double are 
defined as IEEE 754 floating-point number, and NaN/Inf should be valid values; 
Then JSON encoding and the default value encoding are the same; JSON encoding 
should have the same position and allow all valid values.
   
   Therefore, I would say this is not a "new feature", but it's vagueness in 
the spec. We should fix the spec instead of the Rust implementation.


-- 
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

Re: [PR] AVRO-4024: [Rust] support nan/inf/-inf as float/double default [avro]

[via GitHub]

KalleOlaviNiemitalo commented on PR #3051:
URL: https://github.com/apache/avro/pull/3051#issuecomment-2254051260

   > Does this pull request introduce a new feature? no
   
   The "field default values" table at 
 does not 
mention that the default value for the "float,double" Avro types could be 
specified as a JSON string, so, this seems a new feature to me.


-- 
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

Re: [PR] AVRO-3984 [C++] Improve code generated for unions [avro]

[via GitHub]

hwse commented on code in PR #3047:
URL: https://github.com/apache/avro/pull/3047#discussion_r1693255581


##
lang/c++/impl/avrogencpp.cc:
##
@@ -376,8 +392,32 @@ string CodeGen::generateUnionType(const NodePtr ) {
 << "private:\n"
 << "size_t idx_;\n"
 << "std::any value_;\n"
-<< "public:\n"
-<< "size_t idx() const { return idx_; }\n";
+<< "public:\n";
+
+os_ << "/** enum representing union branches as returned by the idx() 
function */\n"
+<< "enum class Branch: size_t {\n";
+
+// generate a enum that maps the branch name to the corresponding index 
(as returned by idx())
+std::set used_branch_names;
+for (size_t i = 0; i < c; ++i) {
+// escape reserved literals for c++
+auto branch_name = decorate(names[i]);
+// avoid rare collisions, e.g. somone might name their struct int_
+if (used_branch_names.find(branch_name) != used_branch_names.end()) {
+size_t postfix = 2;
+std::string escaped_name = branch_name + "_" + 
std::to_string(postfix);
+while (used_branch_names.find(escaped_name) != 
used_branch_names.end()) {
+++postfix;
+escaped_name = branch_name + "_" + std::to_string(postfix);
+}
+branch_name = escaped_name;
+}
+os_ << "" << branch_name << " = " << i << ",\n";
+used_branch_names.insert(branch_name);
+}
+os_ << "};\n";
+
+os_ << "size_t idx() const { return idx_; }\n";

Review Comment:
   Good point, i added the method.



-- 
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

Re: [PR] AVRO-3984 [C++] Improve code generated for unions [avro]

[via GitHub]

hwse commented on code in PR #3047:
URL: https://github.com/apache/avro/pull/3047#discussion_r1693255581


##
lang/c++/impl/avrogencpp.cc:
##
@@ -376,8 +392,32 @@ string CodeGen::generateUnionType(const NodePtr ) {
 << "private:\n"
 << "size_t idx_;\n"
 << "std::any value_;\n"
-<< "public:\n"
-<< "size_t idx() const { return idx_; }\n";
+<< "public:\n";
+
+os_ << "/** enum representing union branches as returned by the idx() 
function */\n"
+<< "enum class Branch: size_t {\n";
+
+// generate a enum that maps the branch name to the corresponding index 
(as returned by idx())
+std::set used_branch_names;
+for (size_t i = 0; i < c; ++i) {
+// escape reserved literals for c++
+auto branch_name = decorate(names[i]);
+// avoid rare collisions, e.g. somone might name their struct int_
+if (used_branch_names.find(branch_name) != used_branch_names.end()) {
+size_t postfix = 2;
+std::string escaped_name = branch_name + "_" + 
std::to_string(postfix);
+while (used_branch_names.find(escaped_name) != 
used_branch_names.end()) {
+++postfix;
+escaped_name = branch_name + "_" + std::to_string(postfix);
+}
+branch_name = escaped_name;
+}
+os_ << "" << branch_name << " = " << i << ",\n";
+used_branch_names.insert(branch_name);
+}
+os_ << "};\n";
+
+os_ << "size_t idx() const { return idx_; }\n";

Review Comment:
   Good point, i added the method. Thanks for the quick feedback!



-- 
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

Re: [PR] AVRO-3984 [C++] Improve code generated for unions [avro]

[via GitHub]

Gerrit0 commented on PR #3047:
URL: https://github.com/apache/avro/pull/3047#issuecomment-2252870876

   > i would not consider the new enum a real feature
   
   I would! It means I can get rid of the enums that are manually maintained 
alongside the avro file today. I think the release note is sufficient 
documentation though, anyone new to using avro will already end up looking at 
the generated header and see it.


-- 
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

Re: [PR] AVRO-3984 [C++] Improve code generated for unions [avro]

[via GitHub]

Gerrit0 commented on code in PR #3047:
URL: https://github.com/apache/avro/pull/3047#discussion_r1693073953


##
lang/c++/impl/avrogencpp.cc:
##
@@ -376,8 +392,32 @@ string CodeGen::generateUnionType(const NodePtr ) {
 << "private:\n"
 << "size_t idx_;\n"
 << "std::any value_;\n"
-<< "public:\n"
-<< "size_t idx() const { return idx_; }\n";
+<< "public:\n";
+
+os_ << "/** enum representing union branches as returned by the idx() 
function */\n"
+<< "enum class Branch: size_t {\n";
+
+// generate a enum that maps the branch name to the corresponding index 
(as returned by idx())
+std::set used_branch_names;
+for (size_t i = 0; i < c; ++i) {
+// escape reserved literals for c++
+auto branch_name = decorate(names[i]);
+// avoid rare collisions, e.g. somone might name their struct int_
+if (used_branch_names.find(branch_name) != used_branch_names.end()) {
+size_t postfix = 2;
+std::string escaped_name = branch_name + "_" + 
std::to_string(postfix);
+while (used_branch_names.find(escaped_name) != 
used_branch_names.end()) {
+++postfix;
+escaped_name = branch_name + "_" + std::to_string(postfix);
+}
+branch_name = escaped_name;
+}
+os_ << "" << branch_name << " = " << i << ",\n";
+used_branch_names.insert(branch_name);
+}
+os_ << "};\n";
+
+os_ << "size_t idx() const { return idx_; }\n";

Review Comment:
   Do you think it is worth including a `branch` method which returns 
`static_cast(idx_)`? In code I have that uses Avro I always end up 
doing this anyways to switch over the enum for branches.



-- 
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

Re: [PR] AVRO-3984 [C++] Improve code generated for unions [avro]

[via GitHub]

mkmkme commented on PR #3047:
URL: https://github.com/apache/avro/pull/3047#issuecomment-2252167056

   > @mkmkme @Gerrit0 
   
   Sorry I'm on a holiday right now and will be available from Monday 


-- 
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

Re: [PR] AVRO-3984 [C++] Improve code generated for unions [avro]

[via GitHub]

martin-g commented on PR #3047:
URL: https://github.com/apache/avro/pull/3047#issuecomment-2252165578

   @mkmkme @Gerrit0 


-- 
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

Re: [PR] Minor: Remove `api` directory from cpp dist [avro]

[via GitHub]

jbonofre commented on PR #3042:
URL: https://github.com/apache/avro/pull/3042#issuecomment-2251261980

   Good catch @Fokko (I fixed a similar issue on `branch-1.11` but forgot to 
cherry-pick). Thanks !


-- 
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

Re: [PR] Minor: Remove `api` directory from cpp dist [avro]

[via GitHub]

martin-g commented on code in PR #3042:
URL: https://github.com/apache/avro/pull/3042#discussion_r1691212368


##
lang/c++/build.sh:
##
@@ -58,7 +58,7 @@ function do_doc() {
 function do_dist() {
   rm -rf $BUILD_CPP/
   mkdir -p $BUILD_CPP
-  cp -r api AUTHORS build.sh CMakeLists.txt ChangeLog \
+  cp -r include/avro AUTHORS build.sh CMakeLists.txt ChangeLog \

Review Comment:
   ```suggestion
 cp -r include AUTHORS build.sh CMakeLists.txt ChangeLog \
   ```
   I hope this is what is expected!
   Now it will copy `include/` with all its subfolders.
   With `cp -r include/avro ...` it will copy just `avro/` and put it in the 
root of $BUILD_CPP. And I guess the missing `include/` will cause problems



-- 
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

Re: [PR] Minor: Remove `api` directory from cpp dist [avro]

[via GitHub]

martin-g commented on code in PR #3042:
URL: https://github.com/apache/avro/pull/3042#discussion_r1691212368


##
lang/c++/build.sh:
##
@@ -58,7 +58,7 @@ function do_doc() {
 function do_dist() {
   rm -rf $BUILD_CPP/
   mkdir -p $BUILD_CPP
-  cp -r api AUTHORS build.sh CMakeLists.txt ChangeLog \
+  cp -r include/avro AUTHORS build.sh CMakeLists.txt ChangeLog \

Review Comment:
   ```suggestion
 cp -r include AUTHORS build.sh CMakeLists.txt ChangeLog \
   ```
   I hope this is what is expected!
   Now it will copy `include/` with all its subfolders.
   With `cp -r include/avro ...` it will copy just `avro/` and put it in the 
root of $BUILD_CPP



-- 
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

Re: [PR] Minor: Remove `api` directory from cpp dist [avro]

[via GitHub]

Fokko commented on code in PR #3042:
URL: https://github.com/apache/avro/pull/3042#discussion_r1690390550


##
lang/c++/build.sh:
##
@@ -58,7 +58,7 @@ function do_doc() {
 function do_dist() {
   rm -rf $BUILD_CPP/
   mkdir -p $BUILD_CPP
-  cp -r api AUTHORS build.sh CMakeLists.txt ChangeLog \
+  cp -r AUTHORS build.sh CMakeLists.txt ChangeLog \

Review Comment:
   I see thanks, I missed that PR  
   ```suggestion
 cp -r include/avro AUTHORS build.sh CMakeLists.txt ChangeLog \
   ```



-- 
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

Re: [PR] Minor: Remove `api` directory from cpp dist [avro]

[via GitHub]

martin-g commented on code in PR #3042:
URL: https://github.com/apache/avro/pull/3042#discussion_r1690365914


##
lang/c++/build.sh:
##
@@ -58,7 +58,7 @@ function do_doc() {
 function do_dist() {
   rm -rf $BUILD_CPP/
   mkdir -p $BUILD_CPP
-  cp -r api AUTHORS build.sh CMakeLists.txt ChangeLog \
+  cp -r AUTHORS build.sh CMakeLists.txt ChangeLog \

Review Comment:
   
https://github.com/apache/avro/commit/8281e610ab5666bb3ae6e22a2cc2b11af0fb9226



-- 
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

Re: [PR] Minor: Remove `api` directory from cpp dist [avro]

[via GitHub]

Fokko commented on code in PR #3042:
URL: https://github.com/apache/avro/pull/3042#discussion_r1690305661


##
lang/c++/build.sh:
##
@@ -58,7 +58,7 @@ function do_doc() {
 function do_dist() {
   rm -rf $BUILD_CPP/
   mkdir -p $BUILD_CPP
-  cp -r api AUTHORS build.sh CMakeLists.txt ChangeLog \
+  cp -r AUTHORS build.sh CMakeLists.txt ChangeLog \

Review Comment:
   Can you elaborate @martin-g I'm not sure if I'm following you



-- 
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

Re: [PR] Minor: Remove `api` directory from cpp dist [avro]

[via GitHub]

martin-g commented on code in PR #3042:
URL: https://github.com/apache/avro/pull/3042#discussion_r1690061402


##
lang/c++/build.sh:
##
@@ -58,7 +58,7 @@ function do_doc() {
 function do_dist() {
   rm -rf $BUILD_CPP/
   mkdir -p $BUILD_CPP
-  cp -r api AUTHORS build.sh CMakeLists.txt ChangeLog \
+  cp -r AUTHORS build.sh CMakeLists.txt ChangeLog \

Review Comment:
   AFAIR it has been replaced with "include/avro" folder



-- 
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

Re: [PR] Minor: Fix versions in `BUILD.md` [avro]

[via GitHub]

martin-g commented on PR #3040:
URL: https://github.com/apache/avro/pull/3040#issuecomment-2247846266

   1.70.0 is the minimum supported version 
   See Cargo.toml > rust-version 


-- 
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

Re: [PR] Minor: Fix versions in `BUILD.md` [avro]

[via GitHub]

Fokko commented on PR #3040:
URL: https://github.com/apache/avro/pull/3040#issuecomment-2247711692

   @martin-g Do you know if we're still at Rust 1.65 as in `BUILD.md`? I 
noticed that we default to 1.75 in the `Dockerfile`.


-- 
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

Re: [PR] AVRO-4023: Move `IdlUtils` into `org.apache.avro.idl` [avro]

[via GitHub]

Fokko commented on PR #3039:
URL: https://github.com/apache/avro/pull/3039#issuecomment-2247701295

   Thanks for the prompt review  


-- 
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

Re: [PR] AVRO-4022: Add docker CI [avro]

[via GitHub]

Fokko commented on PR #3037:
URL: https://github.com/apache/avro/pull/3037#issuecomment-2247369373

   Thanks for the quick review @martin-g


-- 
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

Re: [PR] AVRO-4022: Add docker CI [avro]

[via GitHub]

Fokko commented on code in PR #3037:
URL: https://github.com/apache/avro/pull/3037#discussion_r1689250649


##
build.sh:
##
@@ -351,9 +351,15 @@ do
   ;;
 
 docker-test)
+  if [ -z "$BUILDPLATFORM" ]; then
+export BUILDPLATFORM=$(docker info --format 
"{{.OSType}}/{{.Architecture}}")
+  fi
   tar -cf- share/docker/Dockerfile $DOCKER_EXTRA_CONTEXT |
-DOCKER_BUILDKIT=1 docker build -t avro-test -f share/docker/Dockerfile 
-
-  docker run --rm -v "${PWD}:/avro${DOCKER_MOUNT_FLAG}" --env 
"JAVA=${JAVA:-8}" avro-test /avro/share/docker/run-tests.sh
+DOCKER_BUILDKIT=1 docker build -t avro-test --build-arg 
BUILDPLATFORM="${BUILDPLATFORM}" -f share/docker/Dockerfile -
+  docker run --rm \
+--volume "${PWD}:/avro${DOCKER_MOUNT_FLAG}" \
+--volume "${PWD}/share/docker/m2/:/root/.m2/" \
+--env "JAVA=${JAVA:-8}" avro-test /avro/share/docker/run-tests.sh

Review Comment:
   Good catch, thanks!



-- 
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

Re: [PR] AVRO-4022: Add docker CI [avro]

[via GitHub]

martin-g commented on code in PR #3037:
URL: https://github.com/apache/avro/pull/3037#discussion_r1689000423


##
build.sh:
##
@@ -351,9 +351,15 @@ do
   ;;
 
 docker-test)
+  if [ -z "$BUILDPLATFORM" ]; then
+export BUILDPLATFORM=$(docker info --format 
"{{.OSType}}/{{.Architecture}}")
+  fi
   tar -cf- share/docker/Dockerfile $DOCKER_EXTRA_CONTEXT |
-DOCKER_BUILDKIT=1 docker build -t avro-test -f share/docker/Dockerfile 
-
-  docker run --rm -v "${PWD}:/avro${DOCKER_MOUNT_FLAG}" --env 
"JAVA=${JAVA:-8}" avro-test /avro/share/docker/run-tests.sh
+DOCKER_BUILDKIT=1 docker build -t avro-test --build-arg 
BUILDPLATFORM="${BUILDPLATFORM}" -f share/docker/Dockerfile -
+  docker run --rm \
+--volume "${PWD}:/avro${DOCKER_MOUNT_FLAG}" \
+--volume "${PWD}/share/docker/m2/:/root/.m2/" \
+--env "JAVA=${JAVA:-8}" avro-test /avro/share/docker/run-tests.sh

Review Comment:
   ```suggestion
   --env "JAVA=${JAVA:-11}" avro-test /avro/share/docker/run-tests.sh
   ```
   



-- 
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

Re: [PR] Bump env_logger from 0.10.2 to 0.11.3 in /lang/rust [avro]

[via GitHub]

dependabot[bot] commented on PR #2788:
URL: https://github.com/apache/avro/pull/2788#issuecomment-2245861625

   A newer version of env_logger exists, but since this PR has been edited by 
someone other than Dependabot I haven't updated it. You'll get a PR for the 
updated version as normal once this PR is merged.


-- 
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

Re: [PR] Allow user to define BUILDPLATFORM to build Docker image [avro]

[via GitHub]

Fokko commented on PR #3038:
URL: https://github.com/apache/avro/pull/3038#issuecomment-2245328442

   @jbonofre Thanks for fixing 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-unsubscr...@avro.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] Add docker CI [avro]

[via GitHub]

Fokko commented on code in PR #3037:
URL: https://github.com/apache/avro/pull/3037#discussion_r1688111415


##
share/docker/Dockerfile:
##
@@ -92,6 +92,11 @@ RUN apt-get -qqy install --no-install-recommends libzstd-dev 
\
  php-mbstring \
  php-dev
 
+RUN wget -q https://www.openssl.org/source/openssl-1.1.1q.tar.gz && \

Review Comment:
   Beautiful, thank you!



-- 
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

Re: [PR] Add docker CI [avro]

[via GitHub]

zcsizmadia commented on code in PR #3037:
URL: https://github.com/apache/avro/pull/3037#discussion_r1688077254


##
share/docker/Dockerfile:
##
@@ -92,6 +92,11 @@ RUN apt-get -qqy install --no-install-recommends libzstd-dev 
\
  php-mbstring \
  php-dev
 
+RUN wget -q https://www.openssl.org/source/openssl-1.1.1q.tar.gz && \

Review Comment:
   This should work for multi arch:
   ```
   ARCH="$(dpkg --print-architecture)"
   LIBSSL_DEB="libssl1.1_1.1.1f-1ubuntu2_$ARCH.deb"
   if [ "$ARCH" == "amd64" ] || [ "$ARCH" == "i386" ]; then
   
LIBSSL_URL="http://security.ubuntu.com/ubuntu/pool/main/o/openssl/$LIBSSL_DEB;
   else
   
LIBSSL_URL="http://ports.ubuntu.com/ubuntu-ports/pool/main/o/openssl/$LIBSSL_DEB;
   fi
   wget "$LIBSSL_URL"
   dpkg -i "$LIBSSL_DEB"
   rm -f "$LIBSSL_DEB"
   ```



-- 
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

Re: [PR] Add docker CI [avro]

[via GitHub]

Fokko commented on code in PR #3037:
URL: https://github.com/apache/avro/pull/3037#discussion_r1688032623


##
share/docker/Dockerfile:
##
@@ -92,6 +92,11 @@ RUN apt-get -qqy install --no-install-recommends libzstd-dev 
\
  php-mbstring \
  php-dev
 
+RUN wget -q https://www.openssl.org/source/openssl-1.1.1q.tar.gz && \

Review Comment:
   Yes, the compiling takes quite a while. We should add an if-else to make 
this work for the `arm64` arch's as well



-- 
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

Re: [PR] Add docker CI [avro]

[via GitHub]

zcsizmadia commented on code in PR #3037:
URL: https://github.com/apache/avro/pull/3037#discussion_r1687925912


##
share/docker/Dockerfile:
##
@@ -92,6 +92,11 @@ RUN apt-get -qqy install --no-install-recommends libzstd-dev 
\
  php-mbstring \
  php-dev
 
+RUN wget -q https://www.openssl.org/source/openssl-1.1.1q.tar.gz && \

Review Comment:
   ```
   wget 
http://security.ubuntu.com/ubuntu/pool/main/o/openssl/libssl1.1_1.1.1f-1ubuntu2_amd64.deb
   dpkg -i libssl1.1_1.1.1f-1ubuntu2_amd64.deb
   rm -f libssl1.1_1.1.1f-1ubuntu2_amd64.deb
   ```
   This should work as well, without recompiling openssl1.1.
   
   
   



-- 
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

Re: [PR] Add docker CI [avro]

[via GitHub]

zcsizmadia commented on PR #3037:
URL: https://github.com/apache/avro/pull/3037#issuecomment-2244986039

   Probably we should remove that exception to the console. It is confusing 
that it indicates a hard exception.


-- 
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

Re: [PR] Add docker CI [avro]

[via GitHub]

Fokko commented on PR #3037:
URL: https://github.com/apache/avro/pull/3037#issuecomment-2244977636

   @zcsizmadia Thanks for jumping in here. I was doing some digging myself, and 
I think you're right. It looks like this is because it needs OpenSSL1.1. I 
think this is part of upgrading Ubuntu to 22 which comes with a newer version 
of OpenSSL.


-- 
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

Re: [PR] Add docker CI [avro]

[via GitHub]

zcsizmadia commented on PR #3037:
URL: https://github.com/apache/avro/pull/3037#issuecomment-2244942991

   IIRC those exceptions are expected exceptions for checking an unsupported 
name, but will check later this week. I think the failed test reason is missing 
libssl package.


-- 
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

Re: [PR] AVRO-3968 - [Java] Support for custom @AvroNamespace annotation [avro]

[via GitHub]

henryf1991 commented on PR #2887:
URL: https://github.com/apache/avro/pull/2887#issuecomment-2243403375

   @opwvhk awaiting you review


-- 
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

Re: [PR] AVRO-3631: [Rust] Efficient (de)serialization for optional bytes [avro]

[via GitHub]

martin-g commented on PR #3029:
URL: https://github.com/apache/avro/pull/3029#issuecomment-2238966985

   Thank you, @lerouxrgd !


-- 
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

Re: [PR] AVRO-3631: [Rust] More efficient (de)serialization using serde_bytes [avro]

[via GitHub]

lerouxrgd commented on PR #3027:
URL: https://github.com/apache/avro/pull/3027#issuecomment-2237782945

   @martin-g Actually I've realized that some extras are needed for handling 
types such as `Option>`, I will send a follow-up PR very soon.


-- 
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

Re: [PR] AVRO-3631: [Rust] More efficient (de)serialization using serde_bytes [avro]

[via GitHub]

martin-g commented on PR #3027:
URL: https://github.com/apache/avro/pull/3027#issuecomment-2236547158

   Thank you, @lerouxrgd !


-- 
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

Re: [PR] AVRO-3631: [Rust] More efficient (de)serialization using serde_bytes [avro]

[via GitHub]

martin-g commented on code in PR #3027:
URL: https://github.com/apache/avro/pull/3027#discussion_r1682842169


##
.github/workflows/test-lang-rust-ci-ARM.yml:
##
@@ -31,7 +31,7 @@ permissions:
 
 env:
   RUSTFLAGS: -Dwarnings
-  CARGO_REGISTRIES_CRATES_IO_PROTOCOL: 'git' # TODO: remove this env var once 
MSRV is 1.70.0+
+  CARGO_REGISTRIES_CRATES_IO_PROTOCOL: 'git' # TODO: remove this env var once 
MSRV is 1.73.0+

Review Comment:
   ```suggestion
   ```



##
.github/workflows/test-lang-rust-ci.yml:
##
@@ -31,7 +31,7 @@ permissions:
 
 env:
   RUSTFLAGS: -Dwarnings
-  CARGO_REGISTRIES_CRATES_IO_PROTOCOL: 'git' # TODO: remove this env var once 
MSRV is 1.70.0+
+  CARGO_REGISTRIES_CRATES_IO_PROTOCOL: 'git' # TODO: remove this env var once 
MSRV is 1.73.0+

Review Comment:
   ```suggestion
   ```



-- 
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

Re: [PR] AVRO-3631: [Rust] More efficient (de)serialization using serde_bytes [avro]

[via GitHub]

lerouxrgd commented on PR #3027:
URL: https://github.com/apache/avro/pull/3027#issuecomment-2236468908

   > Please run cargo fmt --all and push again
   
   Done, for some reason I had to use `cargo +nightly fmt --all` for this one.


-- 
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

Re: [PR] AVRO-3631: [Rust] More efficient (de)serialization using serde_bytes [avro]

[via GitHub]

lerouxrgd commented on code in PR #3027:
URL: https://github.com/apache/avro/pull/3027#discussion_r1682811028


##
lang/rust/avro/src/lib.rs:
##
@@ -841,6 +841,7 @@
 //!
 
 mod bigdecimal;
+mod bytes;

Review Comment:
   Indeed, that was my first guess. Then I realized that there is the ambiguous 
serialization to either `Value::Bytes` or `Value::Fixed` that couldn't be 
solved by just using `serde_bytes`. Therefore, I ended up using your approach 
of having a thread local variable to guide the (de)serlializer. With this final 
approach only `serde_avro_{bytes,fixed,slice}` need to be re-exported, as it is 
the case in the PR. 



-- 
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

Re: [PR] AVRO-3631: [Rust] More efficient (de)serialization using serde_bytes [avro]

[via GitHub]

lerouxrgd commented on code in PR #3027:
URL: https://github.com/apache/avro/pull/3027#discussion_r1682805030


##
lang/rust/avro/src/bytes.rs:
##
@@ -0,0 +1,320 @@
+// 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.
+
+use std::cell::Cell;
+
+thread_local! {
+/// A thread local that is used to decide how to serialize Rust bytes into 
an Avro
+/// `types::Value` of type bytes.
+///
+/// Relies on the fact that serde's serialization process is 
single-threaded.
+pub(crate) static SER_BYTES_TYPE: Cell = const { 
Cell::new(BytesType::Bytes) };
+
+/// A thread local that is used to decide how to deserialize an Avro 
`types::Value`
+/// of type bytes into Rust bytes.
+///
+/// Relies on the fact that serde's deserialization process is 
single-threaded.
+pub(crate) static DE_BYTES_BORROWED: Cell = const { Cell::new(false) 
};
+}
+
+#[derive(Debug, Clone, Copy)]
+pub(crate) enum BytesType {
+Bytes,
+Fixed,
+}
+
+/// Efficient (de)serialization of Avro bytes values.
+///
+/// This module is intended to be used through the Serde `with` attribute. See 
below
+/// example:
+///
+/// ```rust
+/// use apache_avro::{serde_avro_bytes, serde_avro_fixed};
+/// use serde::{Deserialize, Serialize};
+///
+/// #[derive(Serialize, Deserialize)]
+/// struct StructWithBytes {
+/// #[serde(with = "serde_avro_bytes")]
+/// vec_field: Vec,
+///
+/// #[serde(with = "serde_avro_fixed")]

Review Comment:
   I was hesitating, but I thought showcasing both in the same struct could 
help a user who would be discovering this feature.



##
lang/rust/avro/src/bytes.rs:
##
@@ -0,0 +1,320 @@
+// 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.
+
+use std::cell::Cell;
+
+thread_local! {
+/// A thread local that is used to decide how to serialize Rust bytes into 
an Avro
+/// `types::Value` of type bytes.
+///
+/// Relies on the fact that serde's serialization process is 
single-threaded.
+pub(crate) static SER_BYTES_TYPE: Cell = const { 
Cell::new(BytesType::Bytes) };
+
+/// A thread local that is used to decide how to deserialize an Avro 
`types::Value`
+/// of type bytes into Rust bytes.
+///
+/// Relies on the fact that serde's deserialization process is 
single-threaded.
+pub(crate) static DE_BYTES_BORROWED: Cell = const { Cell::new(false) 
};
+}
+
+#[derive(Debug, Clone, Copy)]
+pub(crate) enum BytesType {
+Bytes,
+Fixed,
+}
+
+/// Efficient (de)serialization of Avro bytes values.
+///
+/// This module is intended to be used through the Serde `with` attribute. See 
below
+/// example:
+///
+/// ```rust
+/// use apache_avro::{serde_avro_bytes, serde_avro_fixed};
+/// use serde::{Deserialize, Serialize};
+///
+/// #[derive(Serialize, Deserialize)]
+/// struct StructWithBytes {
+/// #[serde(with = "serde_avro_bytes")]
+/// vec_field: Vec,
+///
+/// #[serde(with = "serde_avro_fixed")]
+/// fixed_field: [u8; 6],
+/// }
+/// ```
+pub mod serde_avro_bytes {
+use serde::{Deserializer, Serializer};
+
+pub fn serialize(bytes: &[u8], serializer: S) -> 
Result {
+serde_bytes::serialize(bytes, serializer)
+}
+
+pub fn deserialize<'de, D: Deserializer<'de>>(deserializer: D) -> 
Result, D::Error> {
+serde_bytes::deserialize(deserializer)
+}
+}
+
+/// Efficient (de)serialization of Avro fixed values.
+///
+/// This module is intended to be used through the Serde 

Re: [PR] AVRO-3631: [Rust] More efficient (de)serialization using serde_bytes [avro]

[via GitHub]

lerouxrgd commented on code in PR #3027:
URL: https://github.com/apache/avro/pull/3027#discussion_r1682802382


##
lang/rust/Cargo.toml:
##
@@ -34,7 +34,7 @@ authors = ["Apache Avro team "]
 license = "Apache-2.0"
 repository = "https://github.com/apache/avro;
 edition = "2021"
-rust-version = "1.70.0"
+rust-version = "1.73.0"

Review Comment:
   This is required by `std::thread_local!`'s `LocalKey` when wrapped in a 
`Cell` for having get/set methods available, as you can see [in the 
doc](https://doc.rust-lang.org/std/thread/struct.LocalKey.html#method.get). 
Bumping MSRV was actually advised by clippy.



-- 
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

Re: [PR] AVRO-3631: [Rust] More efficient (de)serialization using serde_bytes [avro]

[via GitHub]

martin-g commented on PR #3027:
URL: https://github.com/apache/avro/pull/3027#issuecomment-2236381110

   Please run `cargo fmt --all` and push again


-- 
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

Re: [PR] AVRO-3631: [Rust] More efficient (de)serialization using serde_bytes [avro]

[via GitHub]

martin-g commented on code in PR #3027:
URL: https://github.com/apache/avro/pull/3027#discussion_r1682746112


##
lang/rust/Cargo.toml:
##
@@ -34,7 +34,7 @@ authors = ["Apache Avro team "]
 license = "Apache-2.0"
 repository = "https://github.com/apache/avro;
 edition = "2021"
-rust-version = "1.70.0"
+rust-version = "1.73.0"

Review Comment:
   According to https://github.com/serde-rs/bytes/blob/0.11.15/Cargo.toml#L12 
the MSRV for serde_bytes is 1.53.
   Why do we need to bump to 1.73.0 here ?



##
lang/rust/avro/src/bytes.rs:
##
@@ -0,0 +1,320 @@
+// 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.
+
+use std::cell::Cell;
+
+thread_local! {
+/// A thread local that is used to decide how to serialize Rust bytes into 
an Avro
+/// `types::Value` of type bytes.
+///
+/// Relies on the fact that serde's serialization process is 
single-threaded.
+pub(crate) static SER_BYTES_TYPE: Cell = const { 
Cell::new(BytesType::Bytes) };
+
+/// A thread local that is used to decide how to deserialize an Avro 
`types::Value`
+/// of type bytes into Rust bytes.
+///
+/// Relies on the fact that serde's deserialization process is 
single-threaded.
+pub(crate) static DE_BYTES_BORROWED: Cell = const { Cell::new(false) 
};
+}
+
+#[derive(Debug, Clone, Copy)]
+pub(crate) enum BytesType {
+Bytes,
+Fixed,
+}
+
+/// Efficient (de)serialization of Avro bytes values.
+///
+/// This module is intended to be used through the Serde `with` attribute. See 
below
+/// example:
+///
+/// ```rust
+/// use apache_avro::{serde_avro_bytes, serde_avro_fixed};
+/// use serde::{Deserialize, Serialize};
+///
+/// #[derive(Serialize, Deserialize)]
+/// struct StructWithBytes {
+/// #[serde(with = "serde_avro_bytes")]
+/// vec_field: Vec,
+///
+/// #[serde(with = "serde_avro_fixed")]

Review Comment:
   Shall we leave only the field for `bytes` here ? Like the doc for `slice` 
below.



##
lang/rust/avro/src/bytes.rs:
##
@@ -0,0 +1,320 @@
+// 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.
+
+use std::cell::Cell;
+
+thread_local! {
+/// A thread local that is used to decide how to serialize Rust bytes into 
an Avro
+/// `types::Value` of type bytes.
+///
+/// Relies on the fact that serde's serialization process is 
single-threaded.
+pub(crate) static SER_BYTES_TYPE: Cell = const { 
Cell::new(BytesType::Bytes) };
+
+/// A thread local that is used to decide how to deserialize an Avro 
`types::Value`
+/// of type bytes into Rust bytes.
+///
+/// Relies on the fact that serde's deserialization process is 
single-threaded.
+pub(crate) static DE_BYTES_BORROWED: Cell = const { Cell::new(false) 
};
+}
+
+#[derive(Debug, Clone, Copy)]
+pub(crate) enum BytesType {
+Bytes,
+Fixed,
+}
+
+/// Efficient (de)serialization of Avro bytes values.
+///
+/// This module is intended to be used through the Serde `with` attribute. See 
below
+/// example:
+///
+/// ```rust
+/// use apache_avro::{serde_avro_bytes, serde_avro_fixed};
+/// use serde::{Deserialize, Serialize};
+///
+/// #[derive(Serialize, Deserialize)]
+/// struct StructWithBytes {
+/// #[serde(with = "serde_avro_bytes")]
+/// vec_field: Vec,
+///
+/// #[serde(with = "serde_avro_fixed")]
+/// fixed_field: [u8; 6],
+/// }
+/// ```
+pub mod serde_avro_bytes {
+use serde::{Deserializer, Serializer};
+
+pub fn serialize(bytes: 

Re: [PR] AVRO-3631: [Rust] Add support for (de)serializing Rust byte arrays to Avro values [avro]

[via GitHub]

lerouxrgd commented on PR #1900:
URL: https://github.com/apache/avro/pull/1900#issuecomment-2236315989

   @martin-g I have prepared GH-3027 for 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-unsubscr...@avro.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] AVRO-3631: [Rust] Add support for (de)serializing Rust byte arrays to Avro values [avro]

[via GitHub]

martin-g commented on PR #1900:
URL: https://github.com/apache/avro/pull/1900#issuecomment-2233350320

   @lerouxrgd 
   0.17 is very close!
   I may not have time to apply it before the cut!
   If you have time to send it as a PR I will find time to review and merge it!


-- 
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

Re: [PR] AVRO-3631: [Rust] Add support for (de)serializing Rust byte arrays to Avro values [avro]

[via GitHub]

lerouxrgd commented on PR #1900:
URL: https://github.com/apache/avro/pull/1900#issuecomment-2233254267

   @martin-g You can apply it manually ! I basically took your unit tests and 
just used `serde_bytes` on the appropriate fields so you should have the 
credits =)


-- 
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

Re: [PR] AVRO-3631: [Rust] Add support for (de)serializing Rust byte arrays to Avro values [avro]

[via GitHub]

martin-g commented on PR #1900:
URL: https://github.com/apache/avro/pull/1900#issuecomment-2233062481

   @lerouxrgd Please send the patch as a Pull Request, so you get your credits!
   Otherwise please let me know and I will apply it manually!


-- 
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

Re: [PR] AVRO-3631: [Rust] Add support for (de)serializing Rust byte arrays to Avro values [avro]

[via GitHub]

lerouxrgd commented on PR #1900:
URL: https://github.com/apache/avro/pull/1900#issuecomment-2231224516

   I had a quick look at this, is there more to it than simply using 
`serde_bytes` on struct fields ?
   
   
   
   Something like this patch
   
   
   ```diff
   diff --git a/lang/rust/Cargo.lock b/lang/rust/Cargo.lock
   index 8fce3d635..572e7bdc0 100644
   --- a/lang/rust/Cargo.lock
   +++ b/lang/rust/Cargo.lock
   @@ -92,6 +92,7 @@ dependencies = [
 "regex-lite",
 "rstest",
 "serde",
   + "serde_bytes",
 "serde_json",
 "serial_test",
 "sha2",
   @@ -1175,6 +1176,15 @@ dependencies = [
 "serde_derive",
]

   +[[package]]
   +name = "serde_bytes"
   +version = "0.11.15"
   +source = "registry+https://github.com/rust-lang/crates.io-index;
   +checksum = 
"387cc504cb06bb40a96c8e04e951fe01854cf6bc921053c954e4a606d9675c6a"
   +dependencies = [
   + "serde",
   +]
   +
[[package]]
name = "serde_derive"
version = "1.0.204"
   diff --git a/lang/rust/Cargo.toml b/lang/rust/Cargo.toml
   index a6fd2ad48..e6b8b5767 100644
   --- a/lang/rust/Cargo.toml
   +++ b/lang/rust/Cargo.toml
   @@ -43,6 +43,7 @@ documentation = "https://docs.rs/apache-avro;
[workspace.dependencies]
log = { default-features = false, version = "0.4.22" }
serde = { default-features = false, version = "1.0.204", features = 
["derive"] }
   +serde_bytes = { default-features = false, version = "0.11.15", features = 
["std"] }
serde_json = { default-features = false, version = "1.0.120", features = 
["std"] }

[profile.release.package.hello-wasm]
   diff --git a/lang/rust/avro/Cargo.toml b/lang/rust/avro/Cargo.toml
   index 27728bcf6..df5874d4e 100644
   --- a/lang/rust/avro/Cargo.toml
   +++ b/lang/rust/avro/Cargo.toml
   @@ -64,6 +64,7 @@ log = { workspace = true }
num-bigint = { default-features = false, version = "0.4.6", features = 
["std", "serde"] }
regex-lite = { default-features = false, version = "0.1.6", features = 
["std", "string"] }
serde = { workspace = true }
   +serde_bytes = { workspace = true }
serde_json = { workspace = true }
snap = { default-features = false, version = "1.1.0", optional = true }
strum = { default-features = false, version = "0.26.3" }
   diff --git a/lang/rust/avro/src/lib.rs b/lang/rust/avro/src/lib.rs
   index a68240600..3c31c1584 100644
   --- a/lang/rust/avro/src/lib.rs
   +++ b/lang/rust/avro/src/lib.rs
   @@ -872,6 +872,7 @@ pub use reader::{
};
pub use schema::{AvroSchema, Schema};
pub use ser::to_value;
   +pub use serde_bytes;
pub use util::{max_allocation_bytes, set_serde_human_readable};
pub use uuid::Uuid;
pub use writer::{
   diff --git a/lang/rust/avro/tests/avro-3631.rs 
b/lang/rust/avro/tests/avro-3631.rs
   new file mode 100644
   index 0..caa361eaf
   --- /dev/null
   +++ b/lang/rust/avro/tests/avro-3631.rs
   @@ -0,0 +1,108 @@
   +// 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.
   +
   +use apache_avro::types::Value;
   +use apache_avro::{from_value, serde_bytes, to_value, Schema};
   +use serde::{Deserialize, Serialize};
   +
   +#[test]
   +fn avro_3631_test_serialize_fixed_fields() {
   +#[derive(Debug, Serialize, Deserialize)]
   +struct TestStructFixedField<'a> {
   +#[serde(with = "serde_bytes")]
   +bytes_field: &'a [u8],
   +
   +#[serde(with = "serde_bytes")]
   +vec_field: Vec,
   +
   +#[serde(with = "serde_bytes")]
   +fixed_field: [u8; 6],
   +}
   +
   +let test = TestStructFixedField {
   +bytes_field: &[1, 2, 3],
   +fixed_field: [1; 6],
   +vec_field: vec![2, 3, 4],
   +};
   +let value: Value = to_value(test).unwrap();
   +let schema = Schema::parse_str(
   +r#"
   +{
   +"type": "record",
   +"name": "TestStructFixedField",
   +"fields": [
   +{
   +"name": "bytes_field",
   +"type": "bytes"
   +},
   +{
   +"name": "vec_field",
   +

Re: [PR] [AVRO-4019] [C++] Turn on even more compiler warnings [avro]

[via GitHub]

martin-g commented on PR #2966:
URL: https://github.com/apache/avro/pull/2966#issuecomment-2230145022

   Thank you, @Gerrit0 !


-- 
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

Re: [PR] [C++] Turn on even more compiler warnings [avro]

[via GitHub]

Gerrit0 commented on code in PR #2966:
URL: https://github.com/apache/avro/pull/2966#discussion_r1678646094


##
lang/c++/include/avro/Validator.hh:
##
@@ -35,7 +35,7 @@ public:
 explicit NullValidator(const ValidSchema &) {}

Review Comment:
   I created  to track this 
issue



-- 
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

Re: [PR] [C++] Turn on even more compiler warnings [avro]

[via GitHub]

martin-g commented on code in PR #2966:
URL: https://github.com/apache/avro/pull/2966#discussion_r1677768902


##
lang/c++/include/avro/Validator.hh:
##
@@ -35,7 +35,7 @@ public:
 explicit NullValidator(const ValidSchema &) {}

Review Comment:
   1.12 is really close to be released!
   The JIRA ticket is needed mostly for the CHANGELOG file, so users are aware 
of the change and can update their code.



-- 
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

Re: [PR] [C++] Turn on even more compiler warnings [avro]

[via GitHub]

martin-g commented on code in PR #2966:
URL: https://github.com/apache/avro/pull/2966#discussion_r1677766834


##
lang/c++/include/avro/buffer/BufferStreambuf.hh:
##
@@ -135,7 +135,11 @@ protected:
 memcpy(c, gptr(), toCopy);
 c += toCopy;
 bytesCopied += toCopy;
-gbump(toCopy);
+while (toCopy > INT_MAX) {

Review Comment:
   ASF Infra team promised to upgrade the Github builders to a newer Ubuntu, 
but there is no timeframe ...



-- 
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

Re: [PR] [C++] Turn on even more compiler warnings [avro]

[via GitHub]

mkmkme commented on code in PR #2966:
URL: https://github.com/apache/avro/pull/2966#discussion_r1677727192


##
lang/c++/include/avro/Validator.hh:
##
@@ -35,7 +35,7 @@ public:
 explicit NullValidator(const ValidSchema &) {}

Review Comment:
   I think yes. Also let's hear from @martin-g as he knows better the release 
cycle.
   
   But at least this should be properly documented as a breaking change.



-- 
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

Re: [PR] [C++] Turn on even more compiler warnings [avro]

[via GitHub]

Gerrit0 commented on code in PR #2966:
URL: https://github.com/apache/avro/pull/2966#discussion_r1677724144


##
lang/c++/include/avro/Validator.hh:
##
@@ -35,7 +35,7 @@ public:
 explicit NullValidator(const ValidSchema &) {}

Review Comment:
   True, I believe the new type is more appropriate here though, as a negative 
value isn't valid for any of these values... does this deserve a Jira issue?



-- 
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

Re: [PR] [C++] Turn on even more compiler warnings [avro]

[via GitHub]

mkmkme commented on code in PR #2966:
URL: https://github.com/apache/avro/pull/2966#discussion_r1677721963


##
lang/c++/include/avro/Reader.hh:
##
@@ -176,15 +176,15 @@ private:
 return encoded;
 }
 
-int64_t readSize() {
+size_t readSize() {

Review Comment:
   Oh sorry, I overlooked it. 



-- 
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

Re: [PR] [C++] Turn on even more compiler warnings [avro]

[via GitHub]

Gerrit0 commented on code in PR #2966:
URL: https://github.com/apache/avro/pull/2966#discussion_r1677721124


##
lang/c++/include/avro/Reader.hh:
##
@@ -176,15 +176,15 @@ private:
 return encoded;
 }
 
-int64_t readSize() {
+size_t readSize() {

Review Comment:
   Both `readSize` and `readCount` are private, I don't believe this change is 
publicly visible (aside from projects with wConversion no longer having to 
suppress warnings when including avro, which is what motivated me to make this 
change)



-- 
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

Re: [PR] [C++] Turn on even more compiler warnings [avro]

[via GitHub]

mkmkme commented on code in PR #2966:
URL: https://github.com/apache/avro/pull/2966#discussion_r1677718795


##
lang/c++/include/avro/buffer/BufferStreambuf.hh:
##
@@ -135,7 +135,11 @@ protected:
 memcpy(c, gptr(), toCopy);
 c += toCopy;
 bytesCopied += toCopy;
-gbump(toCopy);
+while (toCopy > INT_MAX) {

Review Comment:
   Ah I see... thanks!
   You can leave it as it is now and add comment that this is only needed for 
old gcc compatibility.
   
   We do need to upgrade gcc on ARM runner at some point. FYI @martin-g, that's 
not the first time this one bites



-- 
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

Re: [PR] [C++] Turn on even more compiler warnings [avro]

[via GitHub]

Gerrit0 commented on code in PR #2966:
URL: https://github.com/apache/avro/pull/2966#discussion_r1677716799


##
lang/c++/include/avro/buffer/BufferStreambuf.hh:
##
@@ -135,7 +135,11 @@ protected:
 memcpy(c, gptr(), toCopy);
 c += toCopy;
 bytesCopied += toCopy;
-gbump(toCopy);
+while (toCopy > INT_MAX) {

Review Comment:
   I did that first, switched to the macro when gcc 9.4 on the ARM agent was 
complaining about signedness... I can add a `static_cast` instead, hopefully 
newer compilers don't flag that as a useless cast.



-- 
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

Re: [PR] [C++] Turn on even more compiler warnings [avro]

[via GitHub]

Gerrit0 commented on code in PR #2966:
URL: https://github.com/apache/avro/pull/2966#discussion_r1677714491


##
lang/c++/test/buffertest.cc:
##
@@ -34,19 +34,18 @@ using detail::kMinBlockSize;
 using std::cout;
 using std::endl;
 
+// Make a string of repeating 0123456789ABCDEF0123456789...
 std::string makeString(size_t len) {
-std::string newstring;
-newstring.reserve(len);
+std::string result;
+result.reserve(len);
+
+constexpr auto chars = "0123456789ABCDEF";

Review Comment:
   It's resolved to `const char*`, `constexpr auto chars[]` isn't legal, 
switched to `constexpr char chars[]`



-- 
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

Re: [PR] Upgrade jquery to 1.6.3 due to CVE-2011-4969 [avro]

[via GitHub]

Fokko commented on PR #3012:
URL: https://github.com/apache/avro/pull/3012#issuecomment-2228310907

   @Gerrit0 Thank you, you popped up first, sorry for that. Thanks @Astralidea 
 much appreciated!


-- 
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

Re: [PR] Upgrade jquery to 1.6.3 due to CVE-2011-4969 [avro]

[via GitHub]

Gerrit0 commented on PR #3012:
URL: https://github.com/apache/avro/pull/3012#issuecomment-2228285316

   Wasn't me this time - all credit to @Astralidea


-- 
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

Re: [PR] [C++] Turn on even more compiler warnings [avro]

[via GitHub]

mkmkme commented on code in PR #2966:
URL: https://github.com/apache/avro/pull/2966#discussion_r1677524359


##
lang/c++/impl/Node.cc:
##
@@ -146,7 +146,7 @@ void Node::setLogicalType(LogicalType logicalType) {
 if (type_ == AVRO_FIXED) {
 // Max precision that can be supported by the current size of
 // the FIXED type.
-long maxPrecision = floor(log10(2.0) * (8.0 * fixedSize() - 
1));
+int32_t maxPrecision = static_cast(floor(log10(2.0) * 
(8.0 * static_cast(fixedSize()) - 1)));

Review Comment:
   Here definitely `auto`, no need to type the type twice



##
lang/c++/impl/NodeImpl.cc:
##
@@ -577,9 +577,9 @@ NodeMap::NodeMap() : NodeImplMap(AVRO_MAP) {
 
 void NodeUnion::printJson(std::ostream , size_t depth) const {
 os << "[\n";
-int fields = leafAttributes_.size();
+size_t fields = leafAttributes_.size();

Review Comment:
   ditto



##
lang/c++/test/buffertest.cc:
##
@@ -34,19 +34,18 @@ using detail::kMinBlockSize;
 using std::cout;
 using std::endl;
 
+// Make a string of repeating 0123456789ABCDEF0123456789...
 std::string makeString(size_t len) {
-std::string newstring;
-newstring.reserve(len);
+std::string result;
+result.reserve(len);
+
+constexpr auto chars = "0123456789ABCDEF";

Review Comment:
   Not sure how `auto chars` will be resolved in this case, but I'd declare it 
as `constexpr auto chars[]` so that it would be definitely considered an array 
and any possible out-of-bounds `constexpr` access would be caught



##
lang/c++/include/avro/Reader.hh:
##
@@ -176,15 +176,15 @@ private:
 return encoded;
 }
 
-int64_t readSize() {
+size_t readSize() {

Review Comment:
   This one looks a bit scary to me. It introduces a breaking change by 
changing the sign-ness of the return value. If not careful, it can be 
dangerously misrepresented on the user side.



##
lang/c++/impl/FileStream.cc:
##
@@ -94,7 +94,7 @@ struct FileBufferCopyIn : public BufferCopyIn {
 }
 
 bool read(uint8_t *b, size_t toRead, size_t ) final {
-int n = ::read(fd_, b, toRead);
+ssize_t n = ::read(fd_, b, toRead);

Review Comment:
   `auto`? 



##
lang/c++/include/avro/buffer/BufferStreambuf.hh:
##
@@ -135,7 +135,11 @@ protected:
 memcpy(c, gptr(), toCopy);
 c += toCopy;
 bytesCopied += toCopy;
-gbump(toCopy);
+while (toCopy > INT_MAX) {

Review Comment:
   I'd prefer using 
[numeric_limits](https://en.cppreference.com/w/cpp/types/numeric_limits/max) 
here as using macros can be fragile.



##
lang/c++/include/avro/Validator.hh:
##
@@ -35,7 +35,7 @@ public:
 explicit NullValidator(const ValidSchema &) {}

Review Comment:
   This file also introduces user-facing sign-ness changes



##
lang/c++/impl/NodeImpl.cc:
##
@@ -531,9 +531,9 @@ void NodeEnum::printJson(std::ostream , size_t depth) 
const {
 printName(os, nameAttribute_.get(), depth);
 os << indent(depth) << "\"symbols\": [\n";
 
-int names = leafNameAttributes_.size();
+size_t names = leafNameAttributes_.size();

Review Comment:
   `auto` please



##
lang/c++/impl/parsing/ResolvingDecoder.cc:
##
@@ -90,8 +91,8 @@ Symbol ResolvingGrammarGenerator::generate(
 return Symbol::rootSymbol(main, backup);
 }
 
-int ResolvingGrammarGenerator::bestBranch(const NodePtr ,
-  const NodePtr ) {
+std::optional ResolvingGrammarGenerator::bestBranch(const NodePtr 
,

Review Comment:
   That's a really good one, thanks!



-- 
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

Re: [PR] [C++] Turn on even more compiler warnings [avro]

[via GitHub]

mkmkme commented on code in PR #2966:
URL: https://github.com/apache/avro/pull/2966#discussion_r1677519290


##
lang/c++/impl/Compiler.cc:
##
@@ -395,12 +396,12 @@ static NodePtr makeEnumNode(const Entity ,
 
 static NodePtr makeFixedNode(const Entity ,
  const Name , const Object ) {
-int v = static_cast(getLongField(e, m, "size"));
+int64_t v = getLongField(e, m, "size");

Review Comment:
   Maybe even use `auto` here?



-- 
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

Re: [PR] [C++] Turn on even more compiler warnings [avro]

[via GitHub]

mkmkme commented on PR #2966:
URL: https://github.com/apache/avro/pull/2966#issuecomment-2228003979

   > @mkmkme Do you have time to review this PR ?
   
   Yup, thanks for pinging me! I'll do this promptly


-- 
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

Re: [PR] [C++] Turn on even more compiler warnings [avro]

[via GitHub]

martin-g commented on PR #2966:
URL: https://github.com/apache/avro/pull/2966#issuecomment-2228002206

   @mkmkme Do you have time to review this PR ?


-- 
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

Re: [PR] Upgrade jquery to 1.6.3 due to CVE-2011-4969 [avro]

[via GitHub]

Fokko commented on PR #3012:
URL: https://github.com/apache/avro/pull/3012#issuecomment-2227851688

   Thanks @Gerrit0 for fixing 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-unsubscr...@avro.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] Upgrade jquery to 1.6.3 due to CVE-2011-4969 [avro]

[via GitHub]

Gerrit0 commented on code in PR #3012:
URL: https://github.com/apache/avro/pull/3012#discussion_r1677176308


##
lang/java/ipc/src/main/velocity/org/apache/avro/ipc/stats/templates/statsview.vm:
##
@@ -51,7 +51,7 @@
 $title
 
 
-

Review Comment:
   IMO, no. 



-- 
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

Re: [PR] [C++] Turn on even more compiler warnings [avro]

[via GitHub]

Gerrit0 commented on PR #2966:
URL: https://github.com/apache/avro/pull/2966#issuecomment-2227424289

   Well, at this point my theory is that the GCC 9.4 used on ARM (vs 11.4 in 
the other job and 14.1 on my box) doesn't have a properly constexpr'd 
`std::numeric_limits`... for `makeString` I guess the check just got 
smarter, so newer GCC knows it's impossible for that result to change the 
value? Either way, using `INT_MAX` there should make GCC 9.4 happy, and 
`makeString` can be simplified with a character table lookup. *fingers crossed*


-- 
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

Re: [PR] Upgrade jquery to 1.6.3 due to CVE-2011-4969 [avro]

[via GitHub]

Fokko commented on code in PR #3012:
URL: https://github.com/apache/avro/pull/3012#discussion_r1676940337


##
lang/java/ipc/src/main/velocity/org/apache/avro/ipc/stats/templates/statsview.vm:
##
@@ -51,7 +51,7 @@
 $title
 
 
-

Review Comment:
   Shouldn't we pull these resources from a CDN? 
https://code.jquery.com/jquery-1.6.3.min.js
   
   



-- 
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

Re: [PR] AVRO-4004: [Rust] Ignore logicalType fields when creating the canonical form [avro]

[via GitHub]

martin-g commented on code in PR #2976:
URL: https://github.com/apache/avro/pull/2976#discussion_r1675995198


##
lang/rust/avro/src/schema.rs:
##
@@ -3415,16 +3431,16 @@ mod tests {
 
 let schema = Schema::parse_str(raw_schema)?;
 assert_eq!(
-"abf662f831715ff78f88545a05a9262af75d6406b54e1a8a174ff1d2b75affc4",
+"7eb3b28d73dfc99bdd9af1848298b40804a2f8ad5d2642be2ecc2ad34842b987",
 format!("{}", schema.fingerprint::())

Review Comment:
   **Same** - probably not :-/
   There was an initiative by @clesaec and me to add shared tests, e.g. the 
Rust part is at 
https://github.com/apache/avro/blob/main/lang/rust/avro/tests/shared.rs, but it 
is far from exhaustive...



-- 
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