Convert unicode to bytestring in formatCursor

[amaltaro]

Fixes dmwm/DBS#605
Well, another proposal to fix it. This fix goes as close as possible to the SQL/database layer.

I have a couple of concerns though:

• it will apply to every single result calling DBFormatter.formatCursor. So there is some overhead here (complexity O(n^3)!)
• if data is not formatted with one of those methods handling unicode, then there is the risk to pass unicode string data to the next SQL query.

PS.: Oracle returns table/column names in capital, should we apply .lower()? I guess not, there wasn't anything in the previous implementation.

[bbockelm]

This breaks the ability to stream results in the future, no?

[vkuznet]

Alan, I would vote to NOT do it. This code has plenty of issues by itself (e.g. it should not construct objects and put them in a list, instead it should yield results for streaming) and adding additional complexity may have dramatic consequences everywhere (e.g. DBS, T0, etc.)

[amaltaro]

@bbockelm I don't think so. Looking at how it's used in DBS, it expands some rows and return it as a generator:
https://github.com/dmwm/DBS/blob/master/Server/Python/src/dbs/dao/Oracle/Block/List.py#L144

@vkuznet see reply above. In addition to that, this fix does NOT change anything but casting unicode to string. ALL the rest keeps exactly the same logic/behavior as before.

[amaltaro]

Even the additional complexity... it's just more visible now; it was there before, just hidden by the zip function.

[vkuznet]

Alan, I suggest that you benchmark it, take the largest block size and see how much time will be added if we need to parse 10k-100k+ rows. V

…

On 0, Alan Malta Rodrigues ***@***.***> wrote: Even the additional complexity... it's just more visible now; it was there before, just hidden by the `zip` function. -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #9204 (comment)

[bbockelm]

Gross. The whole API is a flurry of object churn, switching between lists and generators.

Honestly - is it better to just either scrap this (introducing a formatCursorV2 that is all generator-based) or force the caller to declare what behavior they need instead of doing it under the hood?

[amaltaro]

Given that we're talking about a hot fix for DBS, I'd add the minimum amount of changes in, the safest the best. Then for the upcoming June release, we try to get an improved version of it.

Yes Valentin, I can do that. I switched gears for the moment and will get back to it later. If @yuyiguo can benchmark it, it's even better.

[vkuznet]

@bbockelm , Brian I proposed full redesign of this and other modules in 2015, see #6337 (and before that here #6284) The changes I made based on pure python based iterators instead of using SQLAlchemy. The code was discussed, tested, merged and put under the rug and never get into the main tree.

@amaltaro , Alan I understand desire to do hot-fix but as history shows the proper solution may not appear and be implemented or put in place.

[bbockelm]

@vkuznet - indeed, it's worthwhile - and important - to go back to this item. However, I am reluctant to do this until the dust has settled.

[amaltaro]

PR probably no longer needed. We shall work on this issue instead:
#9207

and a possible rework between WMCore/DBS for proper use of generators.

[amaltaro]

This was a possible hot fix for DBS and seems to be no longer relevant. Please let me know otherwise such that I can reopen it.