You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@avro.apache.org by "SahilKang (via GitHub)" <gi...@apache.org> on 2024/03/10 23:25:51 UTC
[PR] AVRO-3961: [C] Add AVRO_INVALID to avro_type_t [avro]
SahilKang opened a new pull request, #2799:
URL: https://github.com/apache/avro/pull/2799
## What is the purpose of the change
`EINVAL` was already being cast to an `avro_type_t` in the public interface, and making `AVRO_INVALID` explicit in the enum removes two preexisting workarounds.
`AVRO_INVALID` uses the same value as `EINVAL` for ABI compatibility, and the preexisting enums were given explicit values to help prevent future collisions.
## Verifying this change
This change is already covered by existing tests, such as `make test`
## Documentation
Although `EINVAL` was already being exposed publicly as an `avro_type_t`, this introduces a new symbol so the docs and versions have been updated to reflect a change that:
* adds to the public interface
* maintains ABI compatibility
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-3961: [C] Add AVRO_INVALID to avro_type_t [avro]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g merged PR #2799:
URL: https://github.com/apache/avro/pull/2799
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-3961: [C] Add AVRO_INVALID to avro_type_t [avro]
Posted by "mkmkme (via GitHub)" <gi...@apache.org>.
mkmkme commented on code in PR #2799:
URL: https://github.com/apache/avro/pull/2799#discussion_r1519289885
##########
lang/c/tests/test_avro_type_collision.c:
##########
@@ -0,0 +1,64 @@
+/*
+ * 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
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+ * implied. See the License for the specific language governing
+ * permissions and limitations under the License.
+ */
+
+#include "avro.h"
+
+#include <stdio.h>
+#include <stdlib.h>
+
+#if defined(__GNUC__) || defined(__clang__)
+#pragma GCC diagnostic error "-Wswitch"
+#endif
+
+#define ASSERT_NOT_AVRO_INVALID(type) \
+ if (type == AVRO_INVALID) { \
+ fprintf(stderr, #type " collides with AVRO_INVALID\n"); \
+ exit(EXIT_FAILURE); \
+ } else { \
+ break; \
+ }
+
+#define CASE_ASSERTION(type) case type: ASSERT_NOT_AVRO_INVALID(type)
+
+int main(void)
+{
+ avro_schema_t null_schema = avro_schema_null();
+ avro_type_t type = avro_typeof(null_schema);
+ avro_schema_decref(null_schema);
+
+ switch (type) {
+ CASE_ASSERTION(AVRO_STRING)
+ CASE_ASSERTION(AVRO_BYTES)
+ CASE_ASSERTION(AVRO_INT32)
+ CASE_ASSERTION(AVRO_INT64)
+ CASE_ASSERTION(AVRO_FLOAT)
+ CASE_ASSERTION(AVRO_DOUBLE)
+ CASE_ASSERTION(AVRO_BOOLEAN)
+ CASE_ASSERTION(AVRO_NULL)
+ CASE_ASSERTION(AVRO_RECORD)
+ CASE_ASSERTION(AVRO_ENUM)
+ CASE_ASSERTION(AVRO_FIXED)
+ CASE_ASSERTION(AVRO_MAP)
+ CASE_ASSERTION(AVRO_ARRAY)
+ CASE_ASSERTION(AVRO_UNION)
+ CASE_ASSERTION(AVRO_LINK)
+ case AVRO_INVALID:
+ break;
+ }
Review Comment:
@KalleOlaviNiemitalo I think this should be good enough for the check. If the test can't run due to the duplicate `case`, the CI run will fail and it would mean that the approach should be changed.
WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-3961: [C] Add AVRO_INVALID to avro_type_t [avro]
Posted by "SahilKang (via GitHub)" <gi...@apache.org>.
SahilKang commented on PR #2799:
URL: https://github.com/apache/avro/pull/2799#issuecomment-1987741592
good catch, I updated the commit to set `AVRO_INVALID` to `EINVAL` and added a `lang/c/tests/test_avro_type_collision.c` to verify no collisions are introduced in the future
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-3961: [C] Add AVRO_INVALID to avro_type_t [avro]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on PR #2799:
URL: https://github.com/apache/avro/pull/2799#issuecomment-1991444392
Thank you, @SahilKang, @KalleOlaviNiemitalo & @mkmkme !
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-3961: [C] Add AVRO_INVALID to avro_type_t [avro]
Posted by "KalleOlaviNiemitalo (via GitHub)" <gi...@apache.org>.
KalleOlaviNiemitalo commented on PR #2799:
URL: https://github.com/apache/avro/pull/2799#issuecomment-1987651599
This does not maintain ABI compatibility on all operating systems. The GNU Hurd has `#define EINVAL 0x40000016` rather than plain 22: <https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/mach/hurd/bits/errno.h;h=3b54f5855e21091123ab0669402550968f0746cf;hb=ef321e23c20eebc6d6fb4044425c00e6df27b05f#l247>.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-3961: [C] Add AVRO_INVALID to avro_type_t [avro]
Posted by "mkmkme (via GitHub)" <gi...@apache.org>.
mkmkme commented on PR #2799:
URL: https://github.com/apache/avro/pull/2799#issuecomment-1988137962
@KalleOlaviNiemitalo yeah, me neither, to be honest. But the change looks valid and it doesn't break any stuff in the tests. I'd vote for 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-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-3961: [C] Add AVRO_INVALID to avro_type_t [avro]
Posted by "SahilKang (via GitHub)" <gi...@apache.org>.
SahilKang commented on code in PR #2799:
URL: https://github.com/apache/avro/pull/2799#discussion_r1519296746
##########
lang/c/tests/test_avro_type_collision.c:
##########
@@ -0,0 +1,64 @@
+/*
+ * 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
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+ * implied. See the License for the specific language governing
+ * permissions and limitations under the License.
+ */
+
+#include "avro.h"
+
+#include <stdio.h>
+#include <stdlib.h>
+
+#if defined(__GNUC__) || defined(__clang__)
+#pragma GCC diagnostic error "-Wswitch"
+#endif
+
+#define ASSERT_NOT_AVRO_INVALID(type) \
+ if (type == AVRO_INVALID) { \
+ fprintf(stderr, #type " collides with AVRO_INVALID\n"); \
+ exit(EXIT_FAILURE); \
+ } else { \
+ break; \
+ }
+
+#define CASE_ASSERTION(type) case type: ASSERT_NOT_AVRO_INVALID(type)
+
+int main(void)
+{
+ avro_schema_t null_schema = avro_schema_null();
+ avro_type_t type = avro_typeof(null_schema);
+ avro_schema_decref(null_schema);
+
+ switch (type) {
+ CASE_ASSERTION(AVRO_STRING)
+ CASE_ASSERTION(AVRO_BYTES)
+ CASE_ASSERTION(AVRO_INT32)
+ CASE_ASSERTION(AVRO_INT64)
+ CASE_ASSERTION(AVRO_FLOAT)
+ CASE_ASSERTION(AVRO_DOUBLE)
+ CASE_ASSERTION(AVRO_BOOLEAN)
+ CASE_ASSERTION(AVRO_NULL)
+ CASE_ASSERTION(AVRO_RECORD)
+ CASE_ASSERTION(AVRO_ENUM)
+ CASE_ASSERTION(AVRO_FIXED)
+ CASE_ASSERTION(AVRO_MAP)
+ CASE_ASSERTION(AVRO_ARRAY)
+ CASE_ASSERTION(AVRO_UNION)
+ CASE_ASSERTION(AVRO_LINK)
+ case AVRO_INVALID:
+ break;
+ }
Review Comment:
yeah, and I'd consider such a compilation failure a feature, analogous to a `_Static_assert`
there are other `switch` statements under `src` which will catch the scenario being tested here but I figured it's worth making the edge case explicit as a test
with that said, my goal here is to prevent the other `avro_type_t` branches from aliasing each other: do you have another preference on how to achieve that, or do you think it's better not to test for this at all
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-3961: [C] Add AVRO_INVALID to avro_type_t [avro]
Posted by "KalleOlaviNiemitalo (via GitHub)" <gi...@apache.org>.
KalleOlaviNiemitalo commented on code in PR #2799:
URL: https://github.com/apache/avro/pull/2799#discussion_r1519257266
##########
lang/c/tests/test_avro_type_collision.c:
##########
@@ -0,0 +1,64 @@
+/*
+ * 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
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+ * implied. See the License for the specific language governing
+ * permissions and limitations under the License.
+ */
+
+#include "avro.h"
+
+#include <stdio.h>
+#include <stdlib.h>
+
+#if defined(__GNUC__) || defined(__clang__)
+#pragma GCC diagnostic error "-Wswitch"
+#endif
+
+#define ASSERT_NOT_AVRO_INVALID(type) \
+ if (type == AVRO_INVALID) { \
+ fprintf(stderr, #type " collides with AVRO_INVALID\n"); \
+ exit(EXIT_FAILURE); \
+ } else { \
+ break; \
+ }
+
+#define CASE_ASSERTION(type) case type: ASSERT_NOT_AVRO_INVALID(type)
+
+int main(void)
+{
+ avro_schema_t null_schema = avro_schema_null();
+ avro_type_t type = avro_typeof(null_schema);
+ avro_schema_decref(null_schema);
+
+ switch (type) {
+ CASE_ASSERTION(AVRO_STRING)
+ CASE_ASSERTION(AVRO_BYTES)
+ CASE_ASSERTION(AVRO_INT32)
+ CASE_ASSERTION(AVRO_INT64)
+ CASE_ASSERTION(AVRO_FLOAT)
+ CASE_ASSERTION(AVRO_DOUBLE)
+ CASE_ASSERTION(AVRO_BOOLEAN)
+ CASE_ASSERTION(AVRO_NULL)
+ CASE_ASSERTION(AVRO_RECORD)
+ CASE_ASSERTION(AVRO_ENUM)
+ CASE_ASSERTION(AVRO_FIXED)
+ CASE_ASSERTION(AVRO_MAP)
+ CASE_ASSERTION(AVRO_ARRAY)
+ CASE_ASSERTION(AVRO_UNION)
+ CASE_ASSERTION(AVRO_LINK)
+ case AVRO_INVALID:
+ break;
+ }
Review Comment:
If AVRO_INVALID actually had the same value as e.g. AVRO_ENUM, then this test could not even be run, because the duplicate `case` values in the `switch` would cause compilation to fail.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-3961: [C] Add AVRO_INVALID to avro_type_t [avro]
Posted by "KalleOlaviNiemitalo (via GitHub)" <gi...@apache.org>.
KalleOlaviNiemitalo commented on PR #2799:
URL: https://github.com/apache/avro/pull/2799#issuecomment-1988005805
@mkmkme I have no objection to this PR in its current form, but I don't feel confident in approving it because I'm not familiar with the library.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-3961: [C] Add AVRO_INVALID to avro_type_t [avro]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on PR #2799:
URL: https://github.com/apache/avro/pull/2799#issuecomment-1987793706
@mkmkme CC
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-3961: [C] Add AVRO_INVALID to avro_type_t [avro]
Posted by "KalleOlaviNiemitalo (via GitHub)" <gi...@apache.org>.
KalleOlaviNiemitalo commented on code in PR #2799:
URL: https://github.com/apache/avro/pull/2799#discussion_r1519399160
##########
lang/c/tests/test_avro_type_collision.c:
##########
@@ -0,0 +1,64 @@
+/*
+ * 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
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+ * implied. See the License for the specific language governing
+ * permissions and limitations under the License.
+ */
+
+#include "avro.h"
+
+#include <stdio.h>
+#include <stdlib.h>
+
+#if defined(__GNUC__) || defined(__clang__)
+#pragma GCC diagnostic error "-Wswitch"
+#endif
+
+#define ASSERT_NOT_AVRO_INVALID(type) \
+ if (type == AVRO_INVALID) { \
+ fprintf(stderr, #type " collides with AVRO_INVALID\n"); \
+ exit(EXIT_FAILURE); \
+ } else { \
+ break; \
+ }
+
+#define CASE_ASSERTION(type) case type: ASSERT_NOT_AVRO_INVALID(type)
+
+int main(void)
+{
+ avro_schema_t null_schema = avro_schema_null();
+ avro_type_t type = avro_typeof(null_schema);
+ avro_schema_decref(null_schema);
+
+ switch (type) {
+ CASE_ASSERTION(AVRO_STRING)
+ CASE_ASSERTION(AVRO_BYTES)
+ CASE_ASSERTION(AVRO_INT32)
+ CASE_ASSERTION(AVRO_INT64)
+ CASE_ASSERTION(AVRO_FLOAT)
+ CASE_ASSERTION(AVRO_DOUBLE)
+ CASE_ASSERTION(AVRO_BOOLEAN)
+ CASE_ASSERTION(AVRO_NULL)
+ CASE_ASSERTION(AVRO_RECORD)
+ CASE_ASSERTION(AVRO_ENUM)
+ CASE_ASSERTION(AVRO_FIXED)
+ CASE_ASSERTION(AVRO_MAP)
+ CASE_ASSERTION(AVRO_ARRAY)
+ CASE_ASSERTION(AVRO_UNION)
+ CASE_ASSERTION(AVRO_LINK)
+ case AVRO_INVALID:
+ break;
+ }
Review Comment:
The test is ok, I meant it can be simplified by relying on the compile-time error only and not bothering to compare the values at run time.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org