Обсуждение: Patch: [BUGS] BUG #12320: json parsing with embedded double quotes

On 01/07/2015 08:25 AM, Aaron Botsis wrote:
> Hi folks, I was having a problem importing json data with COPY. Lots
> of things export data nicely as one json blob per line. This is
> excellent for directly importing into a JSON/JSONB column for analysis.
>
> ...Except when there’s an embedded doublequote. Or anything that’s
> escaped. COPY handles this, but by the time the escaped char hit the
> JSON parser, it's not escaped anymore. This breaks the JSON parsing.
> This means I need to manipulate the input data to double-escape it.
> See bug #12320 for an example. Yuck.
>
> I propose this small patch that simply allows specifying COPY … ESCAPE
> without requiring the CSV parser. It will make it much easier to
> directly use json formatted export data for folks going forward. This
> seemed like the simplest route.
>
> Usage is simply:
>
> postgres=# copy t1 from '/Users/nok/Desktop/queries.json';
> ERROR:  invalid input syntax for type json
> DETAIL:  Token "root" is invalid.
> CONTEXT:  JSON data, line 1: ...1418066241619 AND <=1418671041621) AND
> user:"root...
> COPY t1, line 3, column bleh:
> "{"timestamp":"2014-12-15T19:17:32.505Z","duration":7.947,"query":{"query":{"filtered":{"filter":{"qu..."
> postgres=# copy t1 from '/Users/nok/Desktop/queries.json' escape '';
> COPY 1966
>
>


This isn't a bug. Neither CSV format nor TEXT format are partucularly
suitable for json. I'm quite certain I could compose legal json that
will break your proposal (for example, with an embedded newline in the
white space.)

It's also unnecessary. CSV format, while not designed for this, is
nevertheless sufficiently flexible to allow successful import of json
data meeting certain criteria (essentially no newlines), like this:
   copy the_table(jsonfield)   from '/path/to/jsondata'   csv quote e'\x01' delimiter e'\x02';


You aren't the first person to encounter this problem. See
<http://adpgtech.blogspot.com/2014/09/importing-json-data.html>

Maybe we need to add something like this to the docs, or to the wiki.

Note too my comment in that blog post:
   Now this solution is a bit of a hack. I wonder if there's a case for   a COPY mode that simply treats each line as a
singledatum. I also   wonder if we need some more specialized tools for importing JSON,   possibly one or more Foreign
DataWrappers. Such things could   handle, say, embedded newline punctuation. 


cheers

andrew

On 01/08/2015 03:05 PM, Aaron Botsis wrote:
>
>
>> It's also unnecessary. CSV format, while not designed for this, is
>> nevertheless sufficiently flexible to allow successful import of json
>> data meeting certain criteria (essentially no newlines), like this:
>>
>>   copy the_table(jsonfield)
>>   from '/path/to/jsondata'
>>   csv quote e'\x01' delimiter e'\x02’;
>
> While perhaps unnecessary, given the size and simplicity of the patch,
> IMO it’s a no brainer to merge (it actually makes the code smaller by
> 3 lines). It also enables non-json use cases anytime one might want to
> preserve embedded escapes, or use different ones entirely. Do you see
> other reasons not to commit it?


Well, for one thing it's seriously incomplete. You need to be able to
change the delimiter as well. Otherwise, any embedded tab in the json
will cause you major grief.

Currently the delimiter and the escape MUST be a single byte non-nul
character, and there is a check for this in csv mode. Your patch would
allow any arbitrary string (including one of zero length) for the escape
in text mode, and would then silently ignore all but the first byte.
That's not the way we like to do things.

And, frankly, I would need to spend quite a lot more time thinking about
other implications than I have given it so far. This is an area where I
tend to be VERY cautious about making changes. This is a fairly fragile
ecosystem.

cheers

andrew

On 01/08/2015 08:42 PM, Aaron Botsis wrote:
>
>> On Jan 8, 2015, at 3:44 PM, Andrew Dunstan <andrew@dunslane.net> wrote:
>>
>>
>> On 01/08/2015 03:05 PM, Aaron Botsis wrote:
>>>
>>>> It's also unnecessary. CSV format, while not designed for this, is nevertheless sufficiently flexible to allow
successfulimport of json data meeting certain criteria (essentially no newlines), like this: 
>>>>
>>>>   copy the_table(jsonfield)
>>>>   from '/path/to/jsondata'
>>>>   csv quote e'\x01' delimiter e'\x02’;
>>> While perhaps unnecessary, given the size and simplicity of the patch, IMO it’s a no brainer to merge (it actually
makesthe code smaller by 3 lines). It also enables non-json use cases anytime one might want to preserve embedded
escapes,or use different ones entirely. Do you see other reasons not to commit it? 
>>
>> Well, for one thing it's seriously incomplete. You need to be able to change the delimiter as well. Otherwise, any
embeddedtab in the json will cause you major grief. 
>>
>> Currently the delimiter and the escape MUST be a single byte non-nul character, and there is a check for this in csv
mode.Your patch would allow any arbitrary string (including one of zero length) for the escape in text mode, and would
thensilently ignore all but the first byte. That's not the way we like to do things. 
>>
>> And, frankly, I would need to spend quite a lot more time thinking about other implications than I have given it so
far.This is an area where I tend to be VERY cautious about making changes. This is a fairly fragile ecosystem. 
> Good point.
>
> This version:
>
> * doesn't allow ENCODING in BINARY mode (existing bug)
> * doesn’t allow ESCAPE in BINARY mode
> * makes COPY TO work with escape
> * ensures escape char length is < 2 for text mode, 1 for CSV
>
> Couple more things to realize: setting both the escape and delimiter characters to null won’t be any different than
howyou fiddled with them in CSV mode. The code paths will be the same because we should never encounter a null in the
middleof the string. And even if we did (and the encoding didn’t catch it), we’d treat it just like any other field
delimiteror escape character. 
>
> So I’m going to do a bit more testing with another patch tomorrow with delimiters removed. If you can think of any
specificcases you think will break it let me know and I’ll make sure to add regression tests for them as well. 
>
>

Well, I still need convincing that this is the best solution to the
problem. As I said, I need to spend more time thinking about it.

cheers

andrew

> On Jan 8, 2015, at 3:44 PM, Andrew Dunstan <andrew@dunslane.net> wrote:
>
>
> On 01/08/2015 03:05 PM, Aaron Botsis wrote:
>>
>>
>>> It's also unnecessary. CSV format, while not designed for this, is nevertheless sufficiently flexible to allow
successfulimport of json data meeting certain criteria (essentially no newlines), like this: 
>>>
>>>  copy the_table(jsonfield)
>>>  from '/path/to/jsondata'
>>>  csv quote e'\x01' delimiter e'\x02’;
>>
>> While perhaps unnecessary, given the size and simplicity of the patch, IMO it’s a no brainer to merge (it actually
makesthe code smaller by 3 lines). It also enables non-json use cases anytime one might want to preserve embedded
escapes,or use different ones entirely. Do you see other reasons not to commit it? 
>
>
> Well, for one thing it's seriously incomplete. You need to be able to change the delimiter as well. Otherwise, any
embeddedtab in the json will cause you major grief. 
>
> Currently the delimiter and the escape MUST be a single byte non-nul character, and there is a check for this in csv
mode.Your patch would allow any arbitrary string (including one of zero length) for the escape in text mode, and would
thensilently ignore all but the first byte. That's not the way we like to do things. 
>
> And, frankly, I would need to spend quite a lot more time thinking about other implications than I have given it so
far.This is an area where I tend to be VERY cautious about making changes. This is a fairly fragile ecosystem. 

Good point.

This version:

* doesn't allow ENCODING in BINARY mode (existing bug)
* doesn’t allow ESCAPE in BINARY mode
* makes COPY TO work with escape
* ensures escape char length is < 2 for text mode, 1 for CSV

Couple more things to realize: setting both the escape and delimiter characters to null won’t be any different than how
youfiddled with them in CSV mode. The code paths will be the same because we should never encounter a null in the
middleof the string. And even if we did (and the encoding didn’t catch it), we’d treat it just like any other field
delimiteror escape character. 

So I’m going to do a bit more testing with another patch tomorrow with delimiters removed. If you can think of any
specificcases you think will break it let me know and I’ll make sure to add regression tests for them as well.  

Cheers!

Aaron

On Thu, Jan 8, 2015 at 8:42 PM, Aaron Botsis <aaron@bt-r.com> wrote:
> This version:
>
> * doesn't allow ENCODING in BINARY mode (existing bug)
> * doesn’t allow ESCAPE in BINARY mode

Hmm, it does look like ENCODING and ESCAPE don't do anything in BINARY
mode, so adding error checks for that makes sense to me.  I wondered
whether the ENCODING option would affect the interpretation of text
data that might be stored in binary-format COPY data, but I can't find
any evidence that it does.

> * makes COPY TO work with escape

I don't see anything horribly wrong with this, but it seems pretty
useless.  I mean, does anyone really want a newline encoded in the
output as %n or !n or qn rather than \n?  What problem are you trying
to solve here?

> * ensures escape char length is < 2 for text mode, 1 for CSV

So, is the idea here that we're going to have no escape character at
all?  So, the characters that would require the use of an escape
character to represent simply can't be represented at all, but we're
OK with that, because the input data doesn't contain any such
characters?

One general problem with this patch is that it doesn't update the
documentation, which makes it a bit harder than it might be to see
what you're trying to accomplish.

Also, if you want to decrease the chances that this patch will get
forgotten about, you can register it here:

https://commitfest.postgresql.org/action/commitfest_view/open

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company