View Issue Details

IDProjectCategoryView StatusLast Update
0004491JEDI VCL00 JVCL Componentspublic2008-10-26 05:40
ReporterivobauerAssigned ToAHUser 
PrioritynormalSeverityminorReproducibilityalways
Status resolvedResolutionfixed 
Product VersionDaily / GIT 
Target VersionFixed in Version3.36 
Summary0004491: TJvCsvDataSet has issues with floating point values
DescriptionTJvCsvDataSet has some issues with exporting/importing floating point values:

(1) TJvCsvDataSet uses decimal separator from the system to convert floating point values into their string representation. If the decimal separator taken from the system is the same as a CSV field separator (given by TJvCsvDataSet.Separator property), floating point values are not enclosed with quotes during the export, rendering the generated CSV file unusable.

(2) During the import from CSV file, TJvCsvDataSet uses StrToFloatUS on floating point fields in the CSV file to perform conversion between the string representation and the floating point value. In other words, TJvCsvDataSet's import feature is limited to CSV files that use either a dot as the decimal separator or value that is the same as system decimal separator. While not a bug, it severely limits the import capability of otherwise great TJvCsvDataSet.

(3)During the export to CSV file, TJvCsvDataSet deliberately uses the system decimal separator to convert floating point values into their string representation. While not a bug, it severely limits the export capability of otherwise great TJvCsvDataSet.
Steps To Reproduce(1) Set Decimal separator in the Windows Local Settings control panel to a comma (',').

(2) Create a new project in Delphi, drop a TButton on the main form and wire the following code to its OnClick event:

procedure TMainForm.Button1Click(Sender: TObject);
  function GetFullCsvFilePath(const AFileName: string): string;
  begin
    Result :=
IncludeTrailingPathDelimiter(ExtractFilePath(ParamStr(0))) + AFileName;
  end;
var
  LDataSet: TJvCsvDataSet;
begin
  LDataSet := TJvCsvDataSet.Create(nil);
  try
    LDataSet.CsvFieldDef := 'TIME:%,PRESSURE:&';
    LDataSet.FileName := GetFullCsvFilePath('MyCsvTestFile.csv');
    LDataSet.LoadsFromFile := False;
    LDataSet.Open;
    try
      LDataSet.EmptyTable;
      LDataSet.AppendRecord([ 5, 10.01578978]);
      LDataSet.AppendRecord([10, 10.33778987]);
      LDataSet.AppendRecord([15, 10.63823532]);
      LDataSet.AppendRecord([20, 10.85023452]);
      LDataSet.AppendRecord([25, 11.11343532]);
    finally
      LDataSet.Close;
    end;
  finally
    LDataSet.Free;
  end;
end;

(3) Compile and run the program. Here is how the contents of exported CSV file look:

TIME,PRESSURE
5,10,01578978
10,10,33778987
15,10,63823532
20,10,85023452
25,11,11343532
Additional InformationI suggest the following to be done:

(1) For every conversion of floating point values (like import from and export to a CSV file), TJvCsvDataSet should *deliberately* use the decimal separator value taken from a newly introduced read-only property "DecimalSeparator" (see below).

(2) The actual value of DecimalSeparator property depends on the value of the newly introduced boolean property "UseSystemDecimalSeparator". If True, the actual value of decimal separator is to be taken from the system settings. If False, the actual value is taken from the newly introduced property "CustomDecimalSeparator".

(3) I don't know if the capability is in there already, but TJvCsvDataSet should be able to import CSV files that have floating point values enclosed with quotes.

(4) TJvCSvDataSet should be definitely able to detect when the value of its DecimalSeparator property equals to the value of its Separator property and enclose floating point values with quotes during the export to CSV file in such cases.

Here is some pseudo-code that demonstrates points (1) and (2) in this additional information section:

type
  TJvCustomCsvDataSet = class(TDataSet)
    //...
  protected
    //...
    FCustomDecimalSeparator: AnsiChar;
    FUseSystemDecimalSeparator: Boolean;

    function GetDecimalSeparator: AnsiChar;

    property CustomDecimalSeparator: AnsiChar read FCustomDecimalSeparator
      write FCustomDecimalSeparator default '.';

    property DecimalSeparator: Char read GetDecimalSeparator;

    property UseSystemDecimalSeparator: Boolean read FUseSystemDecimalSeparator
      write FUseSystemDecimalSeparator default True;
    //...
  public
    //...
      constructor Create(AOwner: TComponent); override;
    //...
  end;

constructor TJvCustomCsvDataSet.Create(AOwner: TComponent);
begin
  //...
  FCustomDecimalSeparator := '.';
  FUseSystemDecimalSeparator := True;
  //...
end;


function TJvCustomCsvDataSet.GetDecimalSeparator: AnsiChar;
begin
  if UseSystemDecimalSeparator then
    Result := SysUtils.DecimalSeparator
  else
    Result := CustomDecimalSeparator;
end;
TagsNo tags attached.

Activities

obones

2008-10-03 00:16

administrator   ~0014737

I'd rather see something like below, to better mimic what other components are doing.

type
  TJvCustomCsvDataSet = class(TDataSet)
    //...
  protected
    //...
    FDecimalSeparator: AnsiChar;
    FUseSystemDecimalSeparator: Boolean;

    function GetDecimalSeparator: AnsiChar;
    procedure SetDecimalSeparator(const Value: AnsiChar);
    function IsDecimalSeparatorStored: Boolean;

    property DecimalSeparator: Char read GetDecimalSeparator write SetDecimalSeparator stored IsDecimalSeparatorStored;

    property UseSystemDecimalSeparator: Boolean read FUseSystemDecimalSeparator
      write FUseSystemDecimalSeparator default True;
    //...
  public
    //...
      constructor Create(AOwner: TComponent); override;
    //...
  end;

constructor TJvCustomCsvDataSet.Create(AOwner: TComponent);
begin
  //...
  FDecimalSeparator := '.';
  FUseSystemDecimalSeparator := True;
  //...
end;


function TJvCustomCsvDataSet.GetDecimalSeparator: AnsiChar;
begin
  if UseSystemDecimalSeparator then
    Result := SysUtils.DecimalSeparator
  else
    Result := FDecimalSeparator;
end;

procedure TJvCustomCsvDataSet.SetDecimalSeparator(const Value: AnsiChar);
begin
  if FDecimalSeparator <> Value then
  begin
    FDecimalSeparator := Value;
    UseSystemDecimalSeparator := False;
  end;
end;

function TJvCustomCsvDataSet.IsDecimalSeparatorStored: Boolean;
begin
  Result := not UseSystemDecimalSeparator;
end;

wpostma416

2008-10-03 12:56

developer   ~0014739

I am working on solving this. I understand the problem, and I agree with the person logging the original bug that the usefulness of the component is effectively limited by this current behaviour.

In addition, now that I think of it, the software is too fragile in its current state.

I believe that even if the regional settings say that the floating point decimal character is ',', that when encountering a file that contains '1.2', which is the normal CSV float format encountered in programs written by English-speaking programmers, the CSVDataset should parse the floating point number correctly. It is not too hard to make the program handle "1,2" in quotes, and "1.2" in quotes, and nobody (thank goodness!) thus far seems to be using decimal points to separate their CSV files. Whew.

Looking deeply into the routines in JvCsvData.pas, and the "kludges" we have in place in JvJCLUtils, I believe that routines with functionality like FloatToStrUS can not be used anymore, I will implement an alternative solution to the issue that caused someone to write FloatToStrUS and other related units.

This means that once complete, we should remove FloatToStrUS as these "USA-kludge" routines will be orphaned. (They are never called except from JvCsvDataSet).

ivobauer

2008-10-03 16:07

reporter   ~0014742

Olivier: You're right about replacing a default character property value by IsDecimalSeparatorStored method, this is the standard approach for character and string properties. However your implementation stores the property value even if it's the same as the default value set by the constructor, which is not necessary. Therefore I'd suggest the following implementation instead:

function TJvCustomCsvDataSet.IsDecimalSeparatorStored: Boolean;
begin
  Result := not UseSystemDecimalSeparator and (DecimalSeparator <> '.');
end;

ivobauer

2008-10-03 16:42

reporter   ~0014743

Warren: I'd have to disagree with your assertion "file that contains '1.2', which is the normal CSV float format encountered in programs written by English-speaking programmers".

(1) Do you happen to have a link to some standard document that governs the usage of decimal separator in CSV files? I couldn't find one, unfortunately.

(2) I'm aware of the fact that the English is your native language, but IMO it really does not matter what language the programmer speaks. What matters is a client/customer that this programmer is developing their software for. The key here is the *specification* fo requirements for the software that the programer obtains from their client/customer. And those clients/customers can be from all over the world.

(3) I suggest to use the value of DecimalSeparator property *solely* during CSV import when parsing the string representation of floating point values. The users of our applications know the best what is the origin of CSV files they obtained and what kind of decimal separator (and FTR field separator) those CSV files use. Please do not assume anything in your component as it is going to be used in many different scenarios. This means that you can get rid of the FloatToStrUS routine and you don't even need to search for alternative for the purposes of your component.

(4) I don't know whether or not it was obvious from my initial post, but I believe that the value of DecimalSeparator property should also be used *solely* during CSV export when converting floating point values into their string representation.

Warren, please do not take it as personal offense. I'm just trying to demonstrate my points.

wpostma416

2008-10-07 05:42

developer   ~0014774

No personal offense taken. I stand by my original comments.

The CSV standard is used by the vast majority of the programming world, and the vast majority of those users and uses assume a decimal place. There is no standard document, and even if there was, it would be meaningless. To use a german word, 99% of CSV is "gestalt", or to go to latin for a bit, it's "ad-hoc".

Thus, there is no documentation for my assertions. The component is being changed from being decimal-as-period centric to being neutral. None of your later comments helped much with that. Please don't take offense. :-)

W

ivobauer

2008-10-07 06:45

reporter   ~0014776

No personal offense taken. :-)

Just wanted to let you know that I really appreciate your time, effort and willingness to make this component culture-neutral and thus usable outside of your world of decimal point separator. Please do not take it as personal offense. Now I'm just worrying about personal offense infinite recursion problems. :-P

wpostma416

2008-10-07 07:12

developer   ~0014777

Fixed checked in to repository. Revision 11946, on October 7, 2008, 10 AM EST.

ivobauer

2008-10-07 08:01

reporter   ~0014779

Thank you Warren. I promise to test out the new changes ASAP, but it won't be within the next 2 days because I'm just leaving my town.

ivobauer

2008-10-09 14:32

reporter   ~0014798

Warren, I have just finished my tests and your component works exactly as advertised. You did a great work! Thank you so much for these modifications. I think this issue can be marked as resolved now.

Issue History

Date Modified Username Field Change
2008-10-02 14:02 ivobauer New Issue
2008-10-03 00:16 obones Note Added: 0014737
2008-10-03 12:56 wpostma416 Note Added: 0014739
2008-10-03 16:07 ivobauer Note Added: 0014742
2008-10-03 16:42 ivobauer Note Added: 0014743
2008-10-06 00:00 obones Status new => confirmed
2008-10-07 05:42 wpostma416 Note Added: 0014774
2008-10-07 06:45 ivobauer Note Added: 0014776
2008-10-07 07:12 wpostma416 Note Added: 0014777
2008-10-07 08:01 ivobauer Note Added: 0014779
2008-10-09 14:32 ivobauer Note Added: 0014798
2008-10-26 05:39 AHUser Status confirmed => resolved
2008-10-26 05:39 AHUser Fixed in Version => Daily / SVN
2008-10-26 05:39 AHUser Resolution open => fixed
2008-10-26 05:39 AHUser Assigned To => AHUser