bolkedebruin commented on PR #32319: URL: https://github.com/apache/airflow/pull/32319#issuecomment-1826892062
> Also -I think really you are mixing the problem here.... It's not serialize/deserialize we are talking about here. And IMHO the name very well reflects the intention. We are not trying to SERIALIZE/DESERIALIZE things... We are trying to return the value from DBApiHook that can actually be directly serialized. > > So "make_serializable" is a very good name (and intent) it makes whatever gets returned by external thing a common, serializable format.. > > Pretty good and matches the intention. To certain extend, yes, but the signature of the function does not match its intent. > > Why is the common _make_serializable method required? > > For the reason described above - comon "airflow standard" returned output of DBApiHook. All the DBApiHook implementations return some kind of tuple-like output. Some of them also return some kind of description of metadata - sometimes embedded in the returned Row data as extra row content, sometimes not (there is no common standard for that). > Why is this needed and where is this exposed - outside of serialization? If it is only for serialization and the default is to return the same, then imho it should be solved as a serializer. Can we be more specific of what `_make_serializable` is going to return? `Any -> Any` does not cut it. Also if we include header information, which the odbc implementation seems to do, is that part of the specification? Let make sure we then check for that. > And the `make_serializable" returns a standard output that is, well, serializable, in case the originally returned objects are well, not serializable.... This is what custom serializers do. Currently, the implementation here does not enforce that (`Any`). > > Yes - maybe the name could be better but - Naming is the hardest thing in computer science right next to cache invalidation. > :-) > I think at this stage holding release of several providers and comon.sql is quite a bit too much to hold the release for that. I disagree. This is going to be in for a long time, lets make sure we get the design right. > Do you think it is worth it to make extra overhead for the release process? What would be a better name in this case? The name is fine although I could argue for `normalize` or `make_tuple`, if we want to keep the function here. I can see some reasons why, but they are not overly convincing, I would appreciate an argumentation why *not* in `serde`'s serializers. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
