Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add iceberg_avro interface #34

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

add iceberg_avro interface #34

wants to merge 12 commits into from

Conversation

zhjwpku
Copy link
Collaborator

@zhjwpku zhjwpku commented Jan 18, 2025

Add iceberg_avro interface, this closes #17

@zhjwpku zhjwpku force-pushed the add_avro branch 3 times, most recently from 162bac1 to a670e0b Compare January 18, 2025 09:56
@zhjwpku
Copy link
Collaborator Author

zhjwpku commented Jan 18, 2025

@wgtmac Sadly I came across a Windows ci build issue, I don't have a Windows PC, it would be great if you could help to resolve this problem. The problem is two folds:

  1. Windows 2019 does not have zlib installed, I installed it in a way that I'm not sure if it's the right way for gh actions
  2. it has compile error on avro side, I searched the avro ci directory, I did not see any windows build example

The compile error is as below, see [1] for details, [2] says it is probably an optimization error, I could not go further because I don't have a windows PC ;(

D:\a\iceberg-cpp\iceberg-cpp\build\_deps\avro-src\lang\c++\include\avro\NodeImpl.hh(280): fatal error C1001: Internal compiler error. [D:\a\iceberg-cpp\iceberg-cpp\build\_deps\avro-build\avrocpp_s.vcxproj]
  (compiler file 'D:\a\_work\1\s\src\vctools\Compiler\Utc\src\p2\main.c', line 213)
   To work around this problem, try simplifying or changing the program near the locations listed above.

[1] https://github.com/zhjwpku/iceberg-cpp/actions/runs/12842821805/job/35814127788
[2] https://learn.microsoft.com/en-us/cpp/error-messages/compiler-errors-1/fatal-error-c1001?view=msvc-170

This closes apache#17

The avro_unittest is added for testing if libavrocpp works, once
we supported Manifest, this should be removed.

Signed-off-by: Junwang Zhao <[email protected]>
@wgtmac
Copy link
Member

wgtmac commented Jan 18, 2025

Thanks @zhjwpku for working on this! I will have a try early next week.

Update:

  1. I managed to build ZLIB from source and install it to /tmp/zlib on Windows.
  2. Built Avro successfully by running cmake .. -DCMAKE_PREFIX_PATH=/tmp/zlib -DAVRO_BUILD_EXECUTABLES=OFF -DAVRO_BUILD_TESTS=OFF
  3. Built Iceberg successfully by running cmake .. -DCMAKE_PREFIX_PATH=/tmp/zlib.

.github/workflows/test.yml Outdated Show resolved Hide resolved
src/iceberg/CMakeLists.txt Outdated Show resolved Hide resolved
src/iceberg/CMakeLists.txt Outdated Show resolved Hide resolved
@zhjwpku zhjwpku force-pushed the add_avro branch 6 times, most recently from b5a5fc7 to ae99acd Compare January 21, 2025 12:03
Signed-off-by: Junwang Zhao <[email protected]>
Signed-off-by: Junwang Zhao <[email protected]>
@zhjwpku zhjwpku changed the title add libavrocpp_s add iceberg_avro interface Jan 21, 2025
@wgtmac
Copy link
Member

wgtmac commented Jan 24, 2025

I have created a PR against your repo: zhjwpku#3. The CIs are all green on my side: wgtmac#2. Please take a look and merge if it makes sense. @zhjwpku

@zhjwpku
Copy link
Collaborator Author

zhjwpku commented Jan 24, 2025

I have created a PR against your repo: zhjwpku#3. The CIs are all green on my side: wgtmac#2. Please take a look and merge if it makes sense. @zhjwpku

Appreciate for your impressive work, I'll merge it soon.

Copy link
Member

@wgtmac wgtmac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 (Disclaimer: I have pushed some changes to make the CI happy)

Thanks @zhjwpku for taking this work!

This is ready for review @raulcd @lidavidm @gaborkaszab

@wgtmac
Copy link
Member

wgtmac commented Jan 24, 2025

I think this is ready to merge @Xuanwo @Fokko

ci/scripts/build_example.sh Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Discussion] Add Avro library
5 participants