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

Make Velox compatible with fmt >= 9 #7896

Closed
assignUser opened this issue Dec 6, 2023 · 4 comments
Closed

Make Velox compatible with fmt >= 9 #7896

assignUser opened this issue Dec 6, 2023 · 4 comments
Labels
enhancement New feature or request

Comments

@assignUser
Copy link
Collaborator

Description

AFAIK the main issue is fmtlib/fmt#3131

@assignUser assignUser added the enhancement New feature or request label Dec 6, 2023
@majetideepak
Copy link
Collaborator

majetideepak commented Dec 7, 2023

Another reason to upgrade is fmtlib/fmt#1895 (comment)
fixed here fmtlib/fmt#2426
I believe the error described here prestodb/presto#21492 (comment) can be fixed by upgrading.

In file included from /Users/mbasmanova/java/presto/presto-native-execution/velox/velox/external/md5/md5.cpp:28:
In file included from /usr/local/include/folly/Conv.h:124:
In file included from /usr/local/include/folly/Demangle.h:19:
In file included from /usr/local/include/folly/FBString.h:34:
/usr/local/include/fmt/format.h:1775:12: error: expected unqualified-id
  if (std::signbit(value)) {  // value < 0 is false for NaN so use signbit.
           ^
/Library/Developer/CommandLineTools/SDKs/MacOSX14.0.sdk/usr/include/math.h:171:5: note: expanded from macro 'signbit'
    ( sizeof(x) == sizeof(float)  ? __inline_signbitf((float)(x))        \

@assignUser
Copy link
Collaborator Author

While working on the folly bump I incidentally discovered the ostream.h related issues we have to fix. 1. ostream.h is included transitivly via Exceptions.h basically everywhere so just grepping for it only gives 1 hit but that is deceptive.

The issue we have to fix are things like this (or with use of <<):

 struct {
    uint8_t startBitOffset;
    int32_t numBits;
    uint8_t bitOffset;
    int32_t maxSpillLevel;
    int32_t expectedExceeds;

    std::string debugString() const {
      return fmt::format(
          "startBitOffset:{}, numBits:{}, bitOffset:{}, maxSpillLevel:{}, expectedExceeds:{}",
          startBitOffset,
          numBits,
          bitOffset,
          maxSpillLevel,
          expectedExceeds);
    }
  } testSettings[] = {
      {0, 2, 2, 0, true},
      {0, 2, 2, 1, false},
      {0, 2, 4, 0, true},
      {0, 2, 0, -1, false},

The formatting in the unnamed struct causes the error:

Cannot format an argument. To make type T formattable provide a formatter specialization: https://fmt.dev/latest/api.html#udt

What would be your preferred method to fix this? Should we specialize a 'reporter struct' or something like that, that can be used for the unnamed structs like this?

@majetideepak
Copy link
Collaborator

majetideepak commented Dec 8, 2023

| Should we specialize a 'reporter struct' or something like that, that can be used for the unnamed structs like this?

@assignUser Can you elaborate on this proposal? From the link you shared I see that for named structs we need to provide a formatter specialization inherited from ostream_formatter.
How will a reporter struct look like?

@assignUser
Copy link
Collaborator Author

Likely I misunderstood the documentation because my cpp is pretty rudimentary for now.

I'll let you propose a solution :) but feel free to ping me if I can help implement it!

kgpai pushed a commit to kgpai/velox-1 that referenced this issue Jan 18, 2024
Summary:
fmt 8.0.1 is not upwards compatible to 9+ but backwards compatibility should not be an issue from what I could see in the logs and have done during facebookincubator#7700 (newst folly is compatible with fmt 10 but we built it with 8.0.1).

Closes facebookincubator#7896

Pull Request resolved: facebookincubator#7941

Differential Revision: D52880145

Pulled By: kgpai

fbshipit-source-id: f11b62cb69aee0776170e972948321410cfd4802
kgpai pushed a commit to kgpai/velox-1 that referenced this issue Jan 18, 2024
Summary:
Pull Request resolved: facebookincubator#8434

fmt 8.0.1 is not upwards compatible to 9+ but backwards compatibility should not be an issue from what I could see in the logs and have done during facebookincubator#7700 (newst folly is compatible with fmt 10 but we built it with 8.0.1).

Closes facebookincubator#7896

Pull Request resolved: facebookincubator#7941

Differential Revision: D52880145

Pulled By: kgpai

fbshipit-source-id: ae9d58575d416519d8d0447380564083da88b4a3
kgpai pushed a commit to kgpai/velox-1 that referenced this issue Jan 18, 2024
Summary:
Pull Request resolved: facebookincubator#8434

fmt 8.0.1 is not upwards compatible to 9+ but backwards compatibility should not be an issue from what I could see in the logs and have done during facebookincubator#7700 (newst folly is compatible with fmt 10 but we built it with 8.0.1).

Closes facebookincubator#7896

Pull Request resolved: facebookincubator#7941

Differential Revision: D52880145

Pulled By: kgpai

fbshipit-source-id: a3ff1859a0df00a9b49f400665f0b16d6f27acba
kgpai pushed a commit to kgpai/velox-1 that referenced this issue Jan 19, 2024
Summary:
Pull Request resolved: facebookincubator#8434

fmt 8.0.1 is not upwards compatible to 9+ but backwards compatibility should not be an issue from what I could see in the logs and have done during facebookincubator#7700 (newst folly is compatible with fmt 10 but we built it with 8.0.1).

Closes facebookincubator#7896

Pull Request resolved: facebookincubator#7941

Differential Revision: D52880145

Pulled By: kgpai

fbshipit-source-id: ea6797059930351aad170eee19026b183d352fd2
kgpai pushed a commit to kgpai/velox-1 that referenced this issue Jan 19, 2024
Summary:
Pull Request resolved: facebookincubator#8434

fmt 8.0.1 is not upwards compatible to 9+ but backwards compatibility should not be an issue from what I could see in the logs and have done during facebookincubator#7700 (newst folly is compatible with fmt 10 but we built it with 8.0.1).

Closes facebookincubator#7896

Pull Request resolved: facebookincubator#7941

Reviewed By: Yuhta

Differential Revision: D52880145

Pulled By: kgpai

fbshipit-source-id: b0724355657f2303d115da5b82572f1cbf3ada72
kgpai pushed a commit to kgpai/velox-1 that referenced this issue Jan 19, 2024
Summary:
Pull Request resolved: facebookincubator#8434

fmt 8.0.1 is not upwards compatible to 9+ but backwards compatibility should not be an issue from what I could see in the logs and have done during facebookincubator#7700 (newst folly is compatible with fmt 10 but we built it with 8.0.1).

Closes facebookincubator#7896

Pull Request resolved: facebookincubator#7941

Reviewed By: Yuhta

Differential Revision: D52880145

Pulled By: kgpai

fbshipit-source-id: 773c88c41c4b7c786fbf108e4257cfe7c3cc2594
kgpai pushed a commit to kgpai/velox-1 that referenced this issue Jan 19, 2024
Summary:
Pull Request resolved: facebookincubator#8434

fmt 8.0.1 is not upwards compatible to 9+ but backwards compatibility should not be an issue from what I could see in the logs and have done during facebookincubator#7700 (newst folly is compatible with fmt 10 but we built it with 8.0.1).

Closes facebookincubator#7896

Pull Request resolved: facebookincubator#7941

Reviewed By: Yuhta

Differential Revision: D52880145

Pulled By: kgpai

fbshipit-source-id: 68db12f6c67e968562122d324d0a9f9e06ea1f62
kgpai pushed a commit to kgpai/velox-1 that referenced this issue Jan 19, 2024
Summary:
Pull Request resolved: facebookincubator#8434

fmt 8.0.1 is not upwards compatible to 9+ but backwards compatibility should not be an issue from what I could see in the logs and have done during facebookincubator#7700 (newst folly is compatible with fmt 10 but we built it with 8.0.1).

Closes facebookincubator#7896

Pull Request resolved: facebookincubator#7941

Reviewed By: Yuhta

Differential Revision: D52880145

Pulled By: kgpai

fbshipit-source-id: 399ff634da95321a05b342adb2ead9f14792ed5c
kgpai pushed a commit to kgpai/velox-1 that referenced this issue Jan 19, 2024
Summary:
Pull Request resolved: facebookincubator#8434

fmt 8.0.1 is not upwards compatible to 9+ but backwards compatibility should not be an issue from what I could see in the logs and have done during facebookincubator#7700 (newst folly is compatible with fmt 10 but we built it with 8.0.1).

Closes facebookincubator#7896

Pull Request resolved: facebookincubator#7941

Reviewed By: Yuhta

Differential Revision: D52880145

Pulled By: kgpai

fbshipit-source-id: 0c4d1ce4f18eca10f69cea7301cc849680767b95
kgpai pushed a commit to kgpai/velox-1 that referenced this issue Jan 19, 2024
Summary:
Pull Request resolved: facebookincubator#8434

fmt 8.0.1 is not upwards compatible to 9+ but backwards compatibility should not be an issue from what I could see in the logs and have done during facebookincubator#7700 (newst folly is compatible with fmt 10 but we built it with 8.0.1).

Closes facebookincubator#7896

Pull Request resolved: facebookincubator#7941

Reviewed By: Yuhta

Differential Revision: D52880145

Pulled By: kgpai

fbshipit-source-id: 1d8704484a14b9f03ee8c709ed283d5e586d35c4
kgpai pushed a commit to kgpai/velox-1 that referenced this issue Jan 20, 2024
Summary:
Pull Request resolved: facebookincubator#8434

fmt 8.0.1 is not upwards compatible to 9+ but backwards compatibility should not be an issue from what I could see in the logs and have done during facebookincubator#7700 (newst folly is compatible with fmt 10 but we built it with 8.0.1).

Closes facebookincubator#7896

Pull Request resolved: facebookincubator#7941

Reviewed By: Yuhta

Differential Revision: D52880145

Pulled By: kgpai

fbshipit-source-id: 1a1c070399a33d0202b284f045db3ab5081966cb
kgpai pushed a commit to kgpai/velox-1 that referenced this issue Jan 20, 2024
Summary:
Pull Request resolved: facebookincubator#8434

fmt 8.0.1 is not upwards compatible to 9+ but backwards compatibility should not be an issue from what I could see in the logs and have done during facebookincubator#7700 (newst folly is compatible with fmt 10 but we built it with 8.0.1).

Closes facebookincubator#7896

Pull Request resolved: facebookincubator#7941

Reviewed By: Yuhta

Differential Revision: D52880145

Pulled By: kgpai

fbshipit-source-id: e847ab3bb268d950a1d82ad2f6b3cd9d18baa70d
kgpai pushed a commit to kgpai/velox-1 that referenced this issue Jan 20, 2024
Summary:
Pull Request resolved: facebookincubator#8434

fmt 8.0.1 is not upwards compatible to 9+ but backwards compatibility should not be an issue from what I could see in the logs and have done during facebookincubator#7700 (newst folly is compatible with fmt 10 but we built it with 8.0.1).

Closes facebookincubator#7896

Pull Request resolved: facebookincubator#7941

Reviewed By: Yuhta

Differential Revision: D52880145

Pulled By: kgpai

fbshipit-source-id: f49db1561006fdd5c876b28a727f104d5aab4a58
kgpai pushed a commit to kgpai/velox-1 that referenced this issue Jan 20, 2024
Summary:
Pull Request resolved: facebookincubator#8434

fmt 8.0.1 is not upwards compatible to 9+ but backwards compatibility should not be an issue from what I could see in the logs and have done during facebookincubator#7700 (newst folly is compatible with fmt 10 but we built it with 8.0.1).

Closes facebookincubator#7896

Pull Request resolved: facebookincubator#7941

Reviewed By: Yuhta

Differential Revision: D52880145

Pulled By: kgpai

fbshipit-source-id: 4c8aa99b6be6a7dc1d2f4854145830f1254fbd2c
kgpai pushed a commit to kgpai/velox-1 that referenced this issue Jan 20, 2024
Summary:
Pull Request resolved: facebookincubator#8434

fmt 8.0.1 is not upwards compatible to 9+ but backwards compatibility should not be an issue from what I could see in the logs and have done during facebookincubator#7700 (newst folly is compatible with fmt 10 but we built it with 8.0.1).

Closes facebookincubator#7896

Pull Request resolved: facebookincubator#7941

Reviewed By: Yuhta

Differential Revision: D52880145

Pulled By: kgpai

fbshipit-source-id: cf96653f76ff2d3c8c802c8230107515a36cb440
kgpai pushed a commit to kgpai/velox-1 that referenced this issue Jan 20, 2024
Summary:
Pull Request resolved: facebookincubator#8434

fmt 8.0.1 is not upwards compatible to 9+ but backwards compatibility should not be an issue from what I could see in the logs and have done during facebookincubator#7700 (newst folly is compatible with fmt 10 but we built it with 8.0.1).

Closes facebookincubator#7896

Pull Request resolved: facebookincubator#7941

Reviewed By: Yuhta

Differential Revision: D52880145

Pulled By: kgpai

fbshipit-source-id: 8f7e8d8d69ac3260a040eb8ae0b43432c87f6841
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
2 participants