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 From/ToFieldValue classes #78

Open
nikomi opened this issue Sep 22, 2017 · 10 comments
Open

Add From/ToFieldValue classes #78

nikomi opened this issue Sep 22, 2017 · 10 comments

Comments

@nikomi
Copy link
Contributor

nikomi commented Sep 22, 2017

Thanx again for your AMQP library!

Here is something I added to my client which you might find useful:

type FieldEntry = (Text, FieldValue)

toFieldEntry ∷ ToFieldValue a ⇒ T.Text → a → FieldEntry
toFieldEntry k v = (k, toFieldValue v)

This relies on classes From/ToFieldValue:

class FromFieldValue a where
    fromFieldValue ∷ FieldValue → Maybe a

instance FromFieldValue () where
    fromFieldValue FVVoid = Just ()
    fromFieldValue _      = Nothing

instance FromFieldValue Bool where
    fromFieldValue (FVBool b) = Just b
    fromFieldValue _          = Nothing

instance FromFieldValue Int where
    fromFieldValue (FVInt8  i) = Just (fromIntegral i)
    fromFieldValue (FVInt16 i) = Just (fromIntegral i)
    fromFieldValue (FVInt32 i) = Just (fromIntegral i)
    fromFieldValue (FVInt64 i) = Just (fromIntegral i)
    fromFieldValue _      = Nothing

instance FromFieldValue ByteString where
    fromFieldValue (FVByteArray b) = Just b
    fromFieldValue _               = Nothing

instance FromFieldValue T.Text where
    fromFieldValue (FVString t) = Just t
    fromFieldValue _            = Nothing

instance FromFieldValue Float where
    fromFieldValue (FVFloat f)  = Just f
    fromFieldValue (FVDouble f) = Just (double2Float f)
    fromFieldValue _            = Nothing

instance FromFieldValue Double where
    fromFieldValue (FVDouble f) = Just f
    fromFieldValue (FVFloat f)  = Just (float2Double f)
    fromFieldValue _            = Nothing

and

class ToFieldValue a where
    toFieldValue ∷ a → FieldValue

instance ToFieldValue () where
    toFieldValue = const FVVoid

instance ToFieldValue Bool where
    toFieldValue = FVBool

instance ToFieldValue Int where
    toFieldValue = FVInt64 ∘ fromIntegral

instance ToFieldValue ByteString where
    toFieldValue = FVByteArray

instance ToFieldValue T.Text where
    toFieldValue = FVString

instance ToFieldValue Float where
    toFieldValue = FVFloat

instance ToFieldValue Double where
    toFieldValue = FVDouble

The integer handling could definitely be improved but this is all I needed for the moment.

Please feel free to add this to your AMQP library if you think it is useful.

@hreinhardt
Copy link
Owner

Thanks for this code. I'm open to adding it, but I'm unsure about the use-case. In what situations do you need to construct non-trivial field-tables?

@nikomi
Copy link
Contributor Author

nikomi commented Sep 22, 2017

I just see this as a tool not to have to think about types and let the compiler do the thinking instead, regardless if the table is trivial or not.

In our use case we transport quite a bit of information in header fields to be able to use header exchanges.

@hreinhardt
Copy link
Owner

I think I see the appeal.

But it's not clear to me whether some of the assumptions your code makes are generally good; for example your code decodes a FVInt64 to an Int, which may cause overflow on 32-bit architectures. Or your decoding of Double to Float may lose precision.
Now we could change it to only do obvious conversions, making it more correct, but also more verbose to use, kind of defeating the whole purpose of this type-class...

So, I'm not opposed to adding this; I'm just not sure how exactly it should behave. We can keep this issue open and see if other people have a need for this.

@nikomi
Copy link
Contributor Author

nikomi commented Sep 22, 2017

I'm exactly with you on Int and Double - I just didn't have the time to take care of variants I didn't need. If you think this is worth adding we should decide on the following points

  • get rid of Double -> Float
  • keep Float -> Double (probably yes)
  • add instances for Int8/16/32/64
  • Int on 32/64 would to check boundaries (would prefer compile-time checks over run-time checks)
  • what about Integer?
  • what about Word8/16/32/64?
  • what about the other types I ignored?

@nikomi
Copy link
Contributor Author

nikomi commented Sep 22, 2017

Integers could look like this

instance ToFieldValue Int8 where
    toFieldValue = FVInt8

instance ToFieldValue Int16 where
    toFieldValue = FVInt16

instance ToFieldValue Int32 where
    toFieldValue = FVInt32

instance ToFieldValue Int64 where
    toFieldValue = FVInt64

instance ToFieldValue Int where
    toFieldValue i = if (maxBound ∷ Int) > fromIntegral (maxBound ∷ Int32)
                        then FVInt64 (fromIntegral i)
                        else FVInt32 (fromIntegral i)

and this

instance FromFieldValue Int8 where
    fromFieldValue (FVInt8  i) = Just i
    fromFieldValue (FVInt16 i) = if i < fromIntegral (minBound ∷ Int8) || i > fromIntegral (maxBound ∷ Int8) then Nothing else Just (fromIntegral i)
    fromFieldValue (FVInt32 i) = if i < fromIntegral (minBound ∷ Int8) || i > fromIntegral (maxBound ∷ Int8) then Nothing else Just (fromIntegral i)
    fromFieldValue (FVInt64 i) = if i < fromIntegral (minBound ∷ Int8) || i > fromIntegral (maxBound ∷ Int8) then Nothing else Just (fromIntegral i)
    fromFieldValue _      = Nothing

instance FromFieldValue Int16 where
    fromFieldValue (FVInt8  i) = Just (fromIntegral i)
    fromFieldValue (FVInt16 i) = Just i
    fromFieldValue (FVInt32 i) = if i < fromIntegral (minBound ∷ Int16) || i > fromIntegral (maxBound ∷ Int16) then Nothing else Just (fromIntegral i)
    fromFieldValue (FVInt64 i) = if i < fromIntegral (minBound ∷ Int16) || i > fromIntegral (maxBound ∷ Int16) then Nothing else Just (fromIntegral i)
    fromFieldValue _      = Nothing

instance FromFieldValue Int32 where
    fromFieldValue (FVInt8  i) = Just (fromIntegral i)
    fromFieldValue (FVInt16 i) = Just (fromIntegral i)
    fromFieldValue (FVInt32 i) = Just i
    fromFieldValue (FVInt64 i) = if i < fromIntegral (minBound ∷ Int32) || i > fromIntegral (maxBound ∷ Int32) then Nothing else Just (fromIntegral i)
    fromFieldValue _      = Nothing

instance FromFieldValue Int64 where
    fromFieldValue (FVInt8  i) = Just (fromIntegral i)
    fromFieldValue (FVInt16 i) = Just (fromIntegral i)
    fromFieldValue (FVInt32 i) = Just (fromIntegral i)
    fromFieldValue (FVInt64 i) = Just i
    fromFieldValue _      = Nothing

instance FromFieldValue Int where
    fromFieldValue (FVInt8  i) = Just (fromIntegral i)
    fromFieldValue (FVInt16 i) = Just (fromIntegral i)
    fromFieldValue (FVInt32 i) = Just (fromIntegral i)
    fromFieldValue (FVInt64 i) = Just (fromIntegral i)
    fromFieldValue _      = Nothing

-- this is asymetric: ToFieldValue Integer could produce an error
instance FromFieldValue Integer where
    fromFieldValue (FVInt8  i) = Just (fromIntegral i)
    fromFieldValue (FVInt16 i) = Just (fromIntegral i)
    fromFieldValue (FVInt32 i) = Just (fromIntegral i)
    fromFieldValue (FVInt64 i) = Just (fromIntegral i)
    fromFieldValue _      = Nothing

@nikomi
Copy link
Contributor Author

nikomi commented Sep 22, 2017

I made the run-time check on ToFieldValue Int as cheap as possible - we could use the best-fitting Int8/16/32/64 at the cost of multiple comparisons.

I personally would avoid using FromFieldValue Int8/16/32 to avoid such comparisons (Int64 has none) but it's good to have them.

Currently Integer is asymmetric having a FromFieldValue instance but no ToFieldValue which could cause overflows. At first I thought I could use Decimal for very large integers but then I realized I have no clue what Decimal in AMQP represents.

@hreinhardt
Copy link
Owner

You raise some good questions, and I'm myself not sure about some of them. For example, dynamically deciding whether Int should be an FVInt32 or FVInt64 means that compiling the same program on different machines might lead to slightly different run-time behaviour. Whether that's going to be a problem depends on the exact use-case, but it could be confusing.
Similarly, I'm also undecided whether Integer should be supported at all, since philosophically we can't really represent unbounded integers in the field-table.

Overall, I'm still unsure whether this should go into the library, and I'd suggest we wait if other people have a similar need. Right now, my guess would be that you would be the only user ever using that functionality, but I might be wrong. So if you still want to work on it and create a pull-request, I'd merge it.

@nikomi
Copy link
Contributor Author

nikomi commented Sep 22, 2017

Ah, no need to hurry, let's wait for other opinions - I would not want to push something into your library you are not fully confident with.

I'm just used to providing such classes - the first time I have to think about which FV.. constructor I need is the the exact moment I think 'I should be able to write toXyz' here and the 2 classes emerge... does not mean it's the only way (or even a good way) for that matter...

Your thoughts on (in)deterministic behaviour are completely valid, and as a library developer you have to take care to cater for different needs. I on the other hand in this case am a library user and therefore am (almost exclusively) concerned with the system I use it for, hence the different approaches.

@nikomi
Copy link
Contributor Author

nikomi commented Sep 24, 2017

There is one additional point: back in the '80s developers used to save memory by packing values in as few bits/bytes as possible, even using single bits for booleans and 2 or 3 bits for small enums. With the every increasing memory capacity of modern computers the pressure to use the smallest possible int representation has decreased to the point where we usually don't give it thought at all. RabbitMQ on the other hand is used for messaging, so header field values directly contribute not only to memory footprint but also network bandwidth - for people wanting to send gazillions of messages in the shortest possible time reducing bandwidth may be more valuable than cpu cycles, so packing integral values into the smallest possible FVInt8/16/32/64 is a valid approach.

For a generic library like this it's hard to satisfy all these contradictory requirements (short of providing multiple different implementations for the user to select) - I've come to think this might better be left out of the lib for the user to do what's best for his/her project.

@hreinhardt
Copy link
Owner

I think if somebody needed that kind of control about network bandwidth and performance, they probably wouldn't be using Haskell in the first place ;)
But I do agree that it's hard for us to come up with sensible defaults that work for most users.

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

No branches or pull requests

2 participants