patchPSPP - Patches: patch #6302, Fix bugs in GET DATA/TYPE=TXT.

 
 

You are not allowed to post comments on this tracker with your current authentication level.

patch #6302: Fix bugs in GET DATA/TYPE=TXT.

Submitter:  Ben Pfaff <blp>
Submitted:  Mon 03 Dec 2007 06:30:31 AM UTC
   
 
Category:  None Item Group:  None
Status:  Done Assigned to:  blp
Open/Closed:  Closed

Jump to the original submission

Mon 07 Jan 2008 12:48:25 AM UTC, comment #15: 

Since it seems that everyone's in agreement now, I'm closing this patch.

Ben Pfaff <blp>
Group administrator
Sat 08 Dec 2007 07:25:34 AM UTC, comment #14: 

You're right.  I didn't think about the error status.

John Darrington <jmd>
Group administrator
Sat 08 Dec 2007 06:18:18 AM UTC, comment #13: 


>I suggest that casereader_is_empty should take a const argument.


I did consider this, but I decided against it because in fact it can have an effect on the casereader, if it causes an error condition to be raised.  Now, it is difficult to imagine a situation where this is an observable difference to a client--since an error can occur at any time--but a const casereader isn't good for much anyway.

I checked in all these patches, including the one that you provided (and thanks for it).

Ben Pfaff <blp>
Group administrator
Fri 07 Dec 2007 05:29:36 PM UTC, comment #12: 


>The attached patch seems to fix the not-reading-any-data problem
>too.


Looks good.

Ben Pfaff <blp>
Group administrator
Fri 07 Dec 2007 09:31:59 AM UTC, comment #11: 

The attached patch seems to fix the not-reading-any-data problem too.


(file #14583)

John Darrington <jmd>
Group administrator
Fri 07 Dec 2007 09:18:53 AM UTC, comment #10: 

These seem to prevent the crash.

I suggest that casereader_is_empty should take a const argument.


John Darrington <jmd>
Group administrator
Thu 06 Dec 2007 07:04:13 AM UTC, comment #9: 

...and the patches.

(file #14570, file #14571)

Ben Pfaff <blp>
Group administrator
Thu 06 Dec 2007 07:03:28 AM UTC, comment #8: 


>Also, I've noticed that if I run the gui, and run, as seperate operations,
>GET DATA /TYPE=TXT /FILE='/etc/passwd' ...
>EXECUTE.
>FREQUENCIES /VARIABLES=shell.
>then a crash occurs at frequencies.q:686


Here are two patches that fix this problem:

frequencies-segfault-fix.patch, the actual fix: we weren't
initializing data when a casereader happened to be empty.  We
definitely to fix this because a casereader can appear empty due to an
I/O error regardless of whether we check for this in advance.

casegrouper-of-no-cases-should-have-no-groups.patch, a secondary fix:
before, a casegrouper made from a casereader that was empty would have
one case group with zero cases; afterward, such a casegrouper has no
case groups.  (This papers over the problem if applied independently,
but doesn't really fix it for the reason described under the other
patch.)

Comments?


Ben Pfaff <blp>
Group administrator
Thu 06 Dec 2007 06:05:55 AM UTC, comment #7: 

jstover wrote:

>I just tried this syntax in the gui:
>...
>It ran, and created the variables, but no cases appeared in the
>data viewer until I ran
>Are the data supposed to appear in the data editor window right >away? If so, is it a problem with the patch or with the data
>editor?


I wrote:

>I assume that the behavior is the same as for DATA LIST. Is that
>correct?


jmd wrote:

>It would seem not. DATA LIST doesn't require an EXECUTE.


I just tried running both this:

GET DATA /TYPE=TXT /FILE='cars.data' /DELIMITERS=' ' /FIRSTCASE=2
             /VARIABLES=model A8
                        year F4
                        mileage F6
                        price F5
                        type A4
                        age F2.

and this:

data list list file='cars.data'/model (a8) year mileage price * type(a4) age.

and I get the same behavior in the GUI in each case: columns in the datasheet change to reflect the new dictionary, but the data does not change.

jmd wrote:

>For any command which does require EXECUTE, I would expect to >see the "Transformation Pending" label appear in the status bar, >but this doesn't happen for this patch.


Setting the data source is not really adding a transformation, so that is not all that surprising.  It is surprising that the data doesn't get read.  I'll see what I can figure out about that.

Ben Pfaff <blp>
Group administrator
Wed 05 Dec 2007 06:42:49 AM UTC, comment #6: 

I checked this in.  I'm changing the title of the patch to reflect open issues that have been pointed out.  I will deal with them as soon as I can.  (I didn't consider them important enough to postpone checking this in, hope you guys agree.)

>1. I suggest that you put a @xref from the DATA LIST section to
>the GET DATA /TYPE=TXT section of the manual.


Done.

>2. The error messages could be kinder if I try to use GET DATA, but
>forget a mandatory subcommand. Currently, I get simply "Syntax error
>expecting `/' at end of command." but appending a / to the end of the
>command doesn't improve things.


I am not sure what you envision should be done here.  At any rate, I
adjusted the code so that a command such as
  get data/type=txt/file=inline/qualifier='x'/.
now prompts the error message
  error: GET DATA: Syntax error expecting VARIABLES at end of command.
This does seem to be an improvement.

>I hope that /etc/passwd entry doesn't come from any important
>machine!


You can rest assured that the data in it is fake and truncated data
liberally mixed (and I use shadow passwords anyhow).

Ben Pfaff <blp>
Group administrator
Wed 05 Dec 2007 12:39:08 AM UTC, comment #5: 


> I assume that the behavior is the same as for DATA LIST. Is that correct?


It would seem not. DATA LIST doesn't require an EXECUTE.  Neither for that matter does GET DATA /TYPE=GNM nor does GET FILE='thing.sav'.   This follows the way that I remember spss operates.

For any command which does require EXECUTE, I would expect to see the "Transformation Pending" label appear in the status bar, but this doesn't happen for this patch.

Also, I've noticed that if I run the gui, and run, as seperate operations,
 GET DATA /TYPE=TXT /FILE='/etc/passwd' ...
 EXECUTE.
 FREQUENCIES /VARIABLES=shell.
then a crash occurs at frequencies.q:686

I'm not able reproduce this except in the GUI, and it only happens when the commands are run separately.

John Darrington <jmd>
Group administrator
Tue 04 Dec 2007 04:48:34 PM UTC, comment #4: 


>I just tried this syntax in the gui:
>...
>It ran, and created the variables, but no cases appeared in the
>data viewer until I ran
>Are the data supposed to appear in the data editor window right >away? If so, is it a problem with the patch or with the data
>editor?


I assume that the behavior is the same as for DATA LIST.  Is that correct?  (I cannot check for myself at the moment.)  If that is so, then it's a GUI issue.  Probably, by design nothing happens until EXECUTE or something else that reads the data occurs.  We could modify the GUI so that an EXECUTE is implied after any syntax runs.

Ben Pfaff <blp>
Group administrator
Tue 04 Dec 2007 04:01:50 PM UTC, comment #3: 

I just tried this syntax in the gui:

get data /type=txt
        /file='/home/jhs/tmp/traffic.txt'
        /arrangement=delimited
        /firstcase=2
        /delimiters ='\t'
        /variables=speed f3.1
                occupants f2.0
                roll f1.0
                severity f1.0
                event a32.

It ran, and created the variables, but no cases appeared in the data viewer until I ran

FREQUENCIES severity.

Are the data supposed to appear in the data editor window right away? If so, is it a problem with the patch or with the data editor?

Jason H Stover <jstover>
Group Member
Mon 03 Dec 2007 10:13:43 PM UTC, comment #2: 

Works for me.

Jason H Stover <jstover>
Group Member
Mon 03 Dec 2007 07:39:48 AM UTC, comment #1: 

Looks good.

Minor comments:

1. I suggest that you put a @xref from the DATA LIST section to the  GET DATA /TYPE=TXT section of the manual.

2. The error messages could be kinder if I try to use GET DATA, but forget a mandatory subcommand.  Currently, I get simply "Syntax error expecting `/' at end of command." but appending a / to the end of the command doesn't improve things.

I hope that /etc/passwd entry doesn't come from any important machine!

John Darrington <jmd>
Group administrator
Mon 03 Dec 2007 06:30:31 AM UTC, original submission:  

Here is a series of patches that prepare for and then implement GET DATA/TYPE=TXT.  This is a preparatory step for GUI-based textual data file reading.

The patches should be applied in the following order:

data-reader-percent.patch
lex-id-match-n.patch
get-data-separate-command.patch
column-range-base.patch
get-data-type-txt.patch

Each patch has a brief comment at the top describing its purpose.  The final patch is the big one.

Ben Pfaff <blp>
Group administrator

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attached Files
file #14583:  patch added by jmd (13KiB - application/octet-stream)
file #14549:  get-data-type-txt.patch added by blp (97KiB - text/x-diff)
file #14545:  data-reader-percent.patch added by blp (3KiB - text/x-diff)
file #14546:  lex-id-match-n.patch added by blp (3KiB - text/x-diff)
file #14548:  column-range-base.patch added by blp (5KiB - text/x-diff)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by jstover (Posted a comment)
  • -email is unavailable- added by jmd (Posted a comment)
  • -email is unavailable- added by blp (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 14 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2008-01-07 blp Open/ClosedOpen Closed
    2007-12-08 blp StatusReady For Test/Review Done
    2007-12-07 jmd Attached File- Added patch, #14583
    2007-12-06 blp Attached File- Added frequencies-segfault-fix.patch, #14570
        Attached File- Added casegrouper-of-no-cases-should-have-no-groups.patch, #14571
    2007-12-06 blp StatusIn Progress Ready For Test/Review
    2007-12-05 blp StatusReady For Test/Review In Progress
        Assigned toNone blp
        SummaryImplement GET DATA/TYPE=TXT. Fix bugs in GET DATA/TYPE=TXT.
    2007-12-03 blp Attached File- Added get-data-type-txt.patch, #14549
    2007-12-03 blp Attached File- Added data-reader-percent.patch, #14545
        Attached File- Added lex-id-match-n.patch, #14546
        Attached File- Added get-data-separate-command.patch, #14547
        Attached File- Added column-range-base.patch, #14548

    Back to the top

    Powered by Savane 3.13-caa5.
    Corresponding source code