Everything

php

Regarding the latest patch on fgetcsv() (stable branch)

php.internals

Moriyoshi Koizumi

22 years ago

Hi Ilia,

During the verification of bug #26600, I just noticed
fgetcsv() now behaves differently than the previous release 4.3.4.

After a quick examination, I found the change you made
on r-1.279.2.41 is related to this issue.

http://cvs.php.net/diff.php/php-src/ext/standard/file.c?
r1=1.279.2.41&r2=1.279.2.42&ty=h

Before the patch, the test case attached below had given the
following result:

array(3) {
[0]=>
string(1) "a"
[1]=>
string(1) "b"
[2]=>
string(1) "c"
}

Since it was patched, fgetcsv() returns the following:

array(3) {
[0]=>
string(1) "a"
[1]=>
string(2) " b"
[2]=>
string(2) " c"
}

While I think the new behaviour is consistent with the CSV format
used by Microsoft Excel, it'd be a BC problem also.

Test case:
<?php
$file = '/tmp/test.csv';
$fp = fopen($file, 'w');
fwrite($fp, "a, b, c\n");
fclose($fp);
$fp = fopen($file, 'r');
var_dump(fgetcsv($fp, filesize($file)));
fclose($fp);
?>

What do you think of this?

Moriyoshi

Ilia A.

22 years ago

On December 12, 2003 01:36 pm, Moriyoshi Koizumi wrote:

> What do you think of this?

I'll apply a fix momentarily, it wouldn't do to break BC in stable branch.

That said, the whole space trimming behavior seems a little unusual since it
will corrupt content especially if said content contains binary data. IMHO
the data read by fgetcsv() should be fetched in such a manner so that the
original string can be recreated.

Ilia

Rasmus Lerdorf

22 years ago

On Fri, 12 Dec 2003, Ilia Alshanetsky wrote:

> That said, the whole space trimming behavior seems a little unusual since it
> will corrupt content especially if said content contains binary data. IMHO
> the data read by fgetcsv() should be fetched in such a manner so that the
> original string can be recreated.

I agree that it would be a good idea to provide a mechanism to do that,
but at this point I don't think we should be changing the behaviour of
fgetcsv() in neither the stable branch nor the HEAD branch.  I'd add a new
binary-safe version of the function instead for this.  Or an optional arg,
but fgetcsv() already has 2 optional args.

-Rasmus

Moriyoshi Koizumi

22 years ago

On 2003/12/13, at 4:02, Rasmus Lerdorf wrote:

> On Fri, 12 Dec 2003, Ilia Alshanetsky wrote:
>> That said, the whole space trimming behavior seems a little unusual
>> since it
>> will corrupt content especially if said content contains binary data.
>> IMHO
>> the data read by fgetcsv() should be fetched in such a manner so that
>> the
>> original string can be recreated.
>
> I agree that it would be a good idea to provide a mechanism to do that,
> but at this point I don't think we should be changing the behaviour of
> fgetcsv() in neither the stable branch nor the HEAD branch.  I'd add a
> new
> binary-safe version of the function instead for this.  Or an optional
> arg,
> but fgetcsv() already has 2 optional args.

My opinion is basically the same as Ilia's. And I think it'd also be
a good idea to introduce a few more option to modify the escaping
behaviour. Escape characters like \ are treated specially
at the moment, while the de facto specification, of Microsoft,
adopts dubbed-quotes style instead of it. IMO we should be
able to choose the behaviour.

Then, where do we go from here?  Eventually we'll need to change the
spec of its arguments, or add a new function as Rasmus said...

Moriyoshi

Ilia A.

22 years ago

On December 12, 2003 02:02 pm, Rasmus Lerdorf wrote:

> I agree that it would be a good idea to provide a mechanism to do that,
> but at this point I don't think we should be changing the behaviour of
> fgetcsv() in neither the stable branch nor the HEAD branch.  I'd add a new
> binary-safe version of the function instead for this.  Or an optional arg,
> but fgetcsv() already has 2 optional args.

I think we could add another optional argument (bitmask) that could be used to
control various capabilities of fgetcsv(). So, if another tuneable behavior
is necessary it could be easily added without breaking BC.

On a related note I should mention that fgetcsv() in 4.3.X is currently 2.5
times faster then it's equivalent in 5.X.

Ilia

Moriyoshi Koizumi

22 years ago

On 2003/12/13, at 4:42, Ilia Alshanetsky wrote:

> On a related note I should mention that fgetcsv() in 4.3.X is
> currently 2.5
> times faster then it's equivalent in 5.X.

I don't know why you're mentioning this at this time,
but I can say it is a sort of necessary evil :) Because the HEAD
version is capable of handling various encodings, and
less intricate IMO. Rather, I was surprised about that result,
it's only 2.5 times slower :)

Moriyoshi

Ilia A.

22 years ago

On December 12, 2003 02:40 pm, Moriyoshi Koizumi wrote:

> I don't know why you're mentioning this at this time,
> but I can say it is a sort of necessary evil :) Because the HEAD
> version is capable of handling various encodings, and
> less intricate IMO. Rather, I was surprised about that result,
> it's only 2.5 times slower :)

I mentioning this now because we are considering changes to the function in
the development branch, which is a fine time to resolve any deficiencies.

The added functionality, which if I understand correctly is support for
multibyte delimeters and enclosures is great. But it hardly explains a
significant performance disparity I am seeing. I believe much of the problem
can be solved by moving from manual string iteration to one using C library
functions such as memchr(). When parsing non-multibyte text there shouldn't
be more then 10-15% performance loss.
I should mention that benchmarks were made using time utility, so advantages
offered by PHP 5's speedups were discounted. Had they been considered the
speed loss would've been 300% or more.

Ilia

Moriyoshi Koizumi

22 years ago

On 2003/12/13, at 5:09, Ilia Alshanetsky wrote:

> I mentioning this now because we are considering changes to the
> function in
> the development branch, which is a fine time to resolve any
> deficiencies.

Okay, fine :)

> The added functionality, which if I understand correctly is support for
> multibyte delimeters and enclosures is great. But it hardly explains a

The change was not for multibyte delimiters and enclosures. The current
implementation still allows only single-byte characters for the
delimiter
and enclosure. I was able to add such a capability as well, but I didn't
because it appeared to fairly slow it down.

As several multibyte encodings like CP932, CP936, CP949, CP950
and Shift_JIS may map a value in range of 0x40 - 0xfe to the second
byte,
which had been a problem. Therefore we need to check if a octet of
a certain position belongs to a multibyte character or not and this
fact motivated me to bring a scanner-like finite-state machine
implementation into fgetcsv() (and basename()).

See http://www.microsoft.com/globaldev/reference/WinCP.mspx for detail.

> significant performance disparity I am seeing. I believe much of the
> problem
> can be solved by moving from manual string iteration to one using C
> library
> functions such as memchr(). When parsing non-multibyte text there
> shouldn't
> be more then 10-15% performance loss.
> I should mention that benchmarks were made using time utility, so
> advantages
> offered by PHP 5's speedups were discounted. Had they been considered
> the
> speed loss would've been 300% or more.

If we limited the support to UTF-8 or EUC encoding only, we'd be
able to drastically gain much better performance. But it won't
actually solve practical problems where it is in action.

Moriyoshi

Ilia A.

22 years ago

How about we add mb_fgetcsv(), which would have full multi-byte support
(including delimeters). I'd imagine for people who need to parse multi-byte
csv files, full functionality is more important then speed. As for the
fgetcsv() in ext/standard/, we can port the 4.3.X code (copy & paste really)
and let PHP 5 users benefit from a faster fgetcsv() for common applications.
What do you think?

Ilia

Moriyoshi Koizumi

22 years ago

On 2003/12/13, at 5:51, Ilia Alshanetsky wrote:

> How about we add mb_fgetcsv(), which would have full multi-byte support
> (including delimeters). I'd imagine for people who need to parse
> multi-byte
> csv files, full functionality is more important then speed. As for the
> fgetcsv() in ext/standard/, we can port the 4.3.X code (copy & paste
> really)
> and let PHP 5 users benefit from a faster fgetcsv() for common
> applications.
> What do you think?

I disagree, because of the following reasons:

1) Not a few people *actually* use fgetcsv() commonly
with multibyte characters indeed. Regarding this,
applications made by those who don't use
such characters don't (and won't) use multibyte specific
functions and that's the problem. This greatly prevents
them from being portable.
2) IMO speed is not a key factor here. People rather wants
trust-worthy behaviour.
3) fgetcsv() implementation in the stable branch is
now too complicated to add a new feature to
and also hard to maintain. We should be able to
eliminate the mblen() calls for acceptable performance.
See the attached result.

Moriyoshi

p.s. fgetcsv() in the stable branch still seems to segfault with
the attached test case (segfault.php.txt).

[The benchmark result]
My code with mblen() (on php5-csv):

real    0m1.389s
user    0m1.330s
sys     0m0.060s

Ditto without mblen():

real    0m0.396s
user    0m0.350s
sys     0m0.040s

Your code (on php4-csv):

real    0m0.332s
user    0m0.270s
sys     0m0.060s

Ilia A.

22 years ago

On December 12, 2003 04:18 pm, Moriyoshi Koizumi wrote:

> I disagree, because of the following reasons:
>
> 1) Not a few people *actually* use fgetcsv() commonly
>     with multibyte characters indeed. Regarding this,
>     applications made by those who don't use
>     such characters don't (and won't) use multibyte specific
>     functions and that's the problem. This greatly prevents
>     them from being portable.

People have lived without multibyte support in fgetcsv() for many years now,
and I did not see a single request on bugs.php.net for fgetcsv() multi-byte
support. So, while this is certainly useful functionality I do not believe it
is as widely needed as you say it is. We also have a multibyte extension that
already implements multi-byte safe variants of common functions, why make
exception for fgetcsv() and add multibyte code into core?

> 2) IMO speed is not a key factor here. People rather wants
>     trust-worthy behaviour.

When it's a few percent and the changes offer significant improvements yes,
but when were are talking about a performance loss of 250-300% or more then
performance must become a consideration as well.

> 3) fgetcsv() implementation in the stable branch is
>     now too complicated to add a new feature to
>     and also hard to maintain. We should be able to
>     eliminate the mblen() calls for acceptable performance.
>     See the attached result.

What features are we talking about here? The only 2 features I can see we may
wish to add are >1 char long enclosures and separators and the binary thing.
Both of these features would be fairly trivial to add.

> p.s. fgetcsv() in the stable branch still seems to segfault with
>       the attached test case (segfault.php.txt).

Writing a fix now, thanks for the heads-up. If you have any more please let me
know.

Ilia A.

22 years ago

On December 12, 2003 03:15 pm, Moriyoshi Koizumi wrote:

> If we limited the support to UTF-8 or EUC encoding only, we'd be
> able to drastically gain much better performance. But it won't
> actually solve practical problems where it is in action.

Could iconv stream filters be used to convert various encoding (if needed) to
UTF-8 thus addressing the problem?

Ilia

Moriyoshi Koizumi

22 years ago

On 2003/12/13, at 9:56, Ilia Alshanetsky wrote:

> On December 12, 2003 03:15 pm, Moriyoshi Koizumi wrote:
>> If we limited the support to UTF-8 or EUC encoding only, we'd be
>> able to drastically gain much better performance. But it won't
>> actually solve practical problems where it is in action.
>
> Could iconv stream filters be used to convert various encoding (if
> needed) to
> UTF-8 thus addressing the problem?

Actually it might do, but doing so leads to great overheads, because
you have to reconvert the strings to restore the initial form.
Besides the conversion is sometimes irreversible. And even it wouldn't
make sense unless iconv extension becomes built-in.

Moriyoshi

Derick Rethans

22 years ago

On Sat, 13 Dec 2003, Moriyoshi Koizumi wrote:

> On 2003/12/13, at 4:42, Ilia Alshanetsky wrote:
>
> > On a related note I should mention that fgetcsv() in 4.3.X is
> > currently 2.5
> > times faster then it's equivalent in 5.X.
>
> I don't know why you're mentioning this at this time,
> but I can say it is a sort of necessary evil :) Because the HEAD
> version is capable of handling various encodings, and
> less intricate IMO. Rather, I was surprised about that result,
> it's only 2.5 times slower :)

I would call that rather unacceptable actually. Isn't it possible create
a new function for this which handles this MB 'crap' (and the same for
basename) so that we don't have to lose performance because of those
issues?

regards,
Derick

Moriyoshi Koizumi

22 years ago

On 2003/12/13, at 5:45, Derick Rethans wrote:

> I would call that rather unacceptable actually. Isn't it possible
> create
> a new function for this which handles this MB 'crap' (and the same for
> basename) so that we don't have to lose performance because of those
> issues?

Don't you think "crap" sounds too disgusting and inappropriate?
Stop such wording here.

Moriyoshi

Derick Rethans

22 years ago

On Fri, 12 Dec 2003, Derick Rethans wrote:

> On Sat, 13 Dec 2003, Moriyoshi Koizumi wrote:
>
> > On 2003/12/13, at 4:42, Ilia Alshanetsky wrote:
> >
> > > On a related note I should mention that fgetcsv() in 4.3.X is
> > > currently 2.5
> > > times faster then it's equivalent in 5.X.
> >
> > I don't know why you're mentioning this at this time,
> > but I can say it is a sort of necessary evil :) Because the HEAD
> > version is capable of handling various encodings, and
> > less intricate IMO. Rather, I was surprised about that result,
> > it's only 2.5 times slower :)
>
> I would call that rather unacceptable actually. Isn't it possible create
> a new function for this which handles this MB 'crap' (and the same for

"crap" is a poor choice of words, I had no plans to insult you in
anyway. My apologies for that.

Derick

Derick Rethans

22 years ago

On Sat, 13 Dec 2003, Moriyoshi Koizumi wrote:

> What do you think of this?

I think the new behavior is correct, and FYI, Excel's format is "a,b,c"
and not "a, b, c" anyway.

Derick

Moriyoshi Koizumi

22 years ago

On 2003/12/13, at 4:13, Derick Rethans wrote:

> I think the new behavior is correct, and FYI, Excel's format is "a,b,c"
> and not "a, b, c" anyway.

I mean the output could be like "a, b, c" with the leading spaces.
The old code strips such spaces that are not in enclosures,
which I think is somewhat wrong as you say.

Moriyoshi