Repository: frictionlessdata/frictionless-py

Issue:
field_filter modifies order of labels, but not of values
transformation pipeline provides function `field_filter` which is not documented much.

My initial expectation was it would take the table and provide new one, having only the columns with specified names and the order of fields would not change.

This is definitely not the case. Current behavior is:
- order of field names in records is changed according to order of names in `names` argument
- however, the order of field values in records remains the same as before
- and target.schema remains unchanged in regards to order of field names

## test_field_filter.py
```python
from frictionless import Resource, transform, steps


def test_field_filter():
    rec1 = {"first": "v111", "second": "v222"}

    header = list(rec1.keys())
    assert header == ["first", "second"]
    row1_vals = list(rec1.values())
    assert row1_vals == ["v111", "v222"]

    data = [header, row1_vals]

    resource = Resource(data)
    resource.infer()
    assert resource.labels == header

    assert dict(resource.read_rows()[0]) == rec1
    assert resource.validate()["valid"]
    new_header = ["second", "first"]
    target = transform(
        resource,
        steps=[
            steps.field_filter(names=new_header),
            # steps.table_normalize(),
        ],
    )
    target_row1 = target.read_rows()[0]
    # execting order of headers not modified
    assert target.labels == header
    # expecting records keeping it's values under original name
    assert dict(target_row1) == rec1
```
The test fails:
```
    def test_field_filter():
        rec1 = {"first": "v111", "second": "v222"}

        header = list(rec1.keys())
        assert header == ["first", "second"]
        row1_vals = list(rec1.values())
        assert row1_vals == ["v111", "v222"]

        data = [header, row1_vals]

        resource = Resource(data)
        resource.infer()
        assert resource.labels == header

        assert dict(resource.read_rows()[0]) == rec1
        assert resource.validate()["valid"]
        new_header = ["second", "first"]
        target = transform(
            resource,
            steps=[
                steps.field_filter(names=new_header),
                # steps.table_normalize(),
            ],
        )
        target_row1 = target.read_rows()[0]
        # expecting order of headers not modified
>       assert target.labels == header
E       AssertionError: assert ['second', 'first'] == ['first', 'second']
E         At index 0 diff: 'second' != 'first'
E         Use -v to get more diff
```
## Clarifying expected behavior

I came to two possible behaviors which make sense to me

### var1: pure field filter without changing their order
The `field_filter` shall:
- keep order of field names unchanged
- result shall contain only the names, which are passed by `names` argument
- `target.schema` shall change accordingly
- of course, all field values in all records shall remain bound to original field name

### var2: filter and reorder columns
There are situations where reordering columns (field names + their values) could come handy. Typical situation is when someone provides a table with all required columns but their order is mixed.

I guess this is not the behavior, everyone expects, so I would propose adding a new parameter `reorder` with default value `False`

The new signature would be `target.field_select(name list[str], reorder: bool = False)`





Discussion:
vlcinsky: There are three things topics to evaluate

## Shall we tolerate name, which does not exist in a schema?
The `names` can contain a name, which is not present in given schema.

There are options:
- reject such a request
- try to provide reasonable result omitting columns which do not exist
- provide extra parameter `strict_names` controlling how the code reacts

Provided code is using the 2nd option providing mostly usable results. Anyway, it would fail, if all names would be missing in the schema.

## Shall we fix `primaryKey` or `foreignKey` in the schema?
Modified schema might be missing columns which are still used in `primaryKey` schema property.

I think, it is not easy to decide how to fix the `primaryKey` schema property and we shall leave this to the user who could fix the schema on his own as needed.

The same applies to `foreignKey` property.

# Proposed resolution
Consider existing choice acceptable and approve the PR.


Patch:
diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
index e28aa022..52c77e6c 100644
--- a/CONTRIBUTING.md
+++ b/CONTRIBUTING.md
@@ -59,7 +59,14 @@ Frictionless is a Python3.6+ framework, and it uses some common Python tools for
 - formatting: `black`
 - type checking: `mypy` (under construction)
 
-You also need `git` to work on the project, and `make` is recommended. After cloning the repository, we recommend creating a virtual environment and installing the dependencies by following this code:
+You also need `git` to work on the project, and `make` is recommended.
+
+After cloning the repository, you can set up the development environment
+either by creating a virtual environment or a docker container.
+
+### Using a virtual environment
+
+Create a virtual environment and install the dependencies by following this code:
 
 > this will install a `git commit` hook running the tests
 
@@ -73,6 +80,32 @@ alias "frictionless=python -m frictionless"
 
 Note: You may need to run `sudo apt-get install postgresql libpq-dev` on a Debian-based system, because the python Postgres module depends on some postgres CLI tools.
 
+### Using a Docker container
+
+Use the following command to build the container:
+
+```bash
+make docker-setup
+```
+
+This should take care of setting up everything. If the container is
+built without errors, you can then run commands like `make` inside the
+container to accomplish various tasks (see the next section for details).
+
+To make things easier, we can create an alias:
+
+```bash
+alias "frictionless-dev=docker run --rm -v $PWD:/home/frictionless -it frictionless-dev"
+```
+
+Then, for example, to run the tests, we can use:
+
+```bash
+frictionless-dev make test
+```
+
+### Using make
+
 Then you can run various make commands:
 - `make docs` - build the docs
 - `make format` - format source code
diff --git a/Dockerfile b/Dockerfile
new file mode 100644
index 00000000..6ec0e503
--- /dev/null
+++ b/Dockerfile
@@ -0,0 +1,19 @@
+# DESCRIPTION: Frictionless Development Environment
+# BUILD: docker build --rm -t frictionless-dev .
+#
+# For running tests using the container, do:
+#   docker run --rm -v $PWD:/home/frictionless -it frictionless-dev make test
+FROM ubuntu:22.04
+
+RUN apt-get update \
+    && apt-get install -y --no-install-recommends \
+        make \
+        python3-pip \
+        python3-psycopg2
+
+COPY . /home/frictionless
+
+WORKDIR /home/frictionless
+
+RUN pip install wheel \
+    && make install
diff --git a/Makefile b/Makefile
index 5d5b216b..3f488d65 100644
--- a/Makefile
+++ b/Makefile
@@ -1,4 +1,4 @@
-.PHONY: all coverage docs install format github lint release test test-ci
+.PHONY: all coverage docker-setup docs install format github lint release test test-ci
 
 
 PACKAGE := $(shell grep '^PACKAGE =' setup.py | cut -d '"' -f2)
@@ -12,6 +12,9 @@ all:
 coverage:
 	sensible-browser coverage/index.html
 
+docker-setup:
+	docker build --rm -t frictionless-dev .
+
 docs:
 	python docs/build.py
 
diff --git a/docs/guides/guides-overview.md b/docs/guides/guides-overview.md
index 9178851c..a149836e 100644
--- a/docs/guides/guides-overview.md
+++ b/docs/guides/guides-overview.md
@@ -24,6 +24,6 @@ Main Frictionless Framework functionality: describe, extract, validate, and tran
 
 ### Advanced Level
 
-Explor the following section for more advanced guides:
+Explore the following section for more advanced guides:
 - [Mastering the Framework](framework/package-guide.md)
 - [Writing an Extension](extension/design-guide.md)
diff --git a/docs/guides/validation-guide.md b/docs/guides/validation-guide.md
index 58f2ab47..e97fd5cc 100644
--- a/docs/guides/validation-guide.md
+++ b/docs/guides/validation-guide.md
@@ -1262,7 +1262,7 @@ pprint(report.flatten(["rowPosition", "fieldPosition", "code"]))
 
 ## Limit Memory
 
-Frictionless is a streaming engine; usually it is possible to validate terrabytes of data with basically O(1) memory consumption. It means that memory usage doesn't depend on the size of your data making the validation infinitely scalable. For some validation, this is not the case because Frctionless needs to buffer some cells e.g. to check uniqueness. Here memory management can be handy.
+Frictionless is a streaming engine; usually it is possible to validate terabytes of data with basically O(1) memory consumption. It means that memory usage doesn't depend on the size of your data making the validation infinitely scalable. For some validation, this is not the case because Frctionless needs to buffer some cells e.g. to check uniqueness. Here memory management can be handy.
 
 The default memory limit is 1000MB. You can adjust this based on your exact use case. For example, if you're running Frictionless as an API server you might reduce the memory usage. If a validation hits the limit it will not raise a failure - it will return a report with a task error:
 
diff --git a/frictionless/assets/VERSION b/frictionless/assets/VERSION
index c62374aa..70d082eb 100644
--- a/frictionless/assets/VERSION
+++ b/frictionless/assets/VERSION
@@ -1,1 +1,1 @@
-4.40.3
+4.40.4
diff --git a/frictionless/helpers.py b/frictionless/helpers.py
index 8686b11e..345d5edf 100644
--- a/frictionless/helpers.py
+++ b/frictionless/helpers.py
@@ -305,6 +305,14 @@ def unzip_descriptor(path, innerpath):
             tempdir = tempfile.mkdtemp()
             zip.extractall(tempdir)
             atexit.register(shutil.rmtree, tempdir)
+            if innerpath == "":
+                innerpath = "datapackage.json"
+                extensions = ("json", "yaml", "yml")
+                default_names = (f"datapackage.{ext}" for ext in extensions)
+                for name in default_names:
+                    if os.path.isfile(os.path.join(tempdir, name)):
+                        innerpath = name
+                        break
             descriptor = os.path.join(tempdir, innerpath)
     return descriptor
 
diff --git a/frictionless/package/package.py b/frictionless/package/package.py
index 5cc1ca9e..2ac5134a 100644
--- a/frictionless/package/package.py
+++ b/frictionless/package/package.py
@@ -99,7 +99,7 @@ class Package(Metadata):
         innerpath? (str): A ZIP datapackage descriptor inner path.
             Path to the package descriptor inside the ZIP datapackage.
             Example: some/folder/datapackage.yaml
-            Default: datapackage.json
+            Default: datapackage.json, datapackage.yaml or datapackage.yml
 
         basepath? (str): A basepath of the resource
             The fullpath of the resource is joined `basepath` and /path`
@@ -154,7 +154,7 @@ class Package(Metadata):
         image=None,
         created=None,
         # Extra
-        innerpath="datapackage.json",
+        innerpath="",
         basepath="",
         detector=None,
         onerror="ignore",
diff --git a/frictionless/plugins/csv/dialect.py b/frictionless/plugins/csv/dialect.py
index 3993b399..2d288290 100644
--- a/frictionless/plugins/csv/dialect.py
+++ b/frictionless/plugins/csv/dialect.py
@@ -141,7 +141,6 @@ class CsvDialect(Dialect):
 
     metadata_profile = {  # type: ignore
         "type": "object",
-        "additionalProperties": False,
         "properties": {
             "delimiter": {"type": "string"},
             "lineTerminator": {"type": "string"},
diff --git a/frictionless/steps/field/field_filter.py b/frictionless/steps/field/field_filter.py
index a966f081..a262e2d7 100644
--- a/frictionless/steps/field/field_filter.py
+++ b/frictionless/steps/field/field_filter.py
@@ -19,11 +19,14 @@ class field_filter(Step):
 
     def transform_resource(self, resource):
         table = resource.to_petl()
-        names = self.get("names")
-        for name in resource.schema.field_names:
-            if name not in names:
-                resource.schema.remove_field(name)
-        resource.data = table.cut(*names)
+        names = self.get("names", [])
+        schema_fields_dict = {dct["name"]: dct for dct in resource.schema.fields}
+        new_schema_fields = [
+            schema_fields_dict[name] for name in names if name in schema_fields_dict
+        ]
+        resource.schema.fields = new_schema_fields
+        new_names = [dct["name"] for dct in new_schema_fields]
+        resource.data = table.cut(*new_names)
 
     # Metadata
 


Test patch:
diff --git a/tests/package/test_general.py b/tests/package/test_general.py
index fbb7114e..f5954a23 100644
--- a/tests/package/test_general.py
+++ b/tests/package/test_general.py
@@ -186,6 +186,13 @@ def test_package_from_zip_innerpath():
     assert len(package.resources) == 10
 
 
+def test_package_from_zip_innerpath_yaml():
+    # for issue1174
+    package = Package("data/innerpath.package.zip")
+    assert package.name == "emissions"
+    assert len(package.resources) == 10
+
+
 @pytest.mark.parametrize("create_descriptor", [(False,), (True,)])
 def test_package_standard_specs_properties(create_descriptor):
     options = dict(
diff --git a/tests/resource/test_dialect.py b/tests/resource/test_dialect.py
index 0e8fac06..2c7c63d5 100644
--- a/tests/resource/test_dialect.py
+++ b/tests/resource/test_dialect.py
@@ -109,6 +109,7 @@ def test_resource_dialect_json_property():
         ]
 
 
+@pytest.mark.skip
 def test_resource_dialect_bad_property():
     resource = Resource("data/table.csv", dialect={"bad": True})
     with pytest.raises(FrictionlessException) as excinfo:
diff --git a/tests/steps/field/test_field_filter.py b/tests/steps/field/test_field_filter.py
index 56075fda..f5b7dbc2 100644
--- a/tests/steps/field/test_field_filter.py
+++ b/tests/steps/field/test_field_filter.py
@@ -1,25 +1,133 @@
+import pytest
+
+
 from frictionless import Resource, transform, steps
 
 
 # General
 
 
-def test_step_field_filter():
-    source = Resource(path="data/transform.csv")
+@pytest.mark.parametrize(
+    "scenario",
+    [
+        {
+            "test_id": "idem",
+            "csv_path": "data/transform.csv",
+            "names": ["id", "name", "population"],
+            "expected_schema": {
+                "fields": [
+                    {"name": "id", "type": "integer"},
+                    {"name": "name", "type": "string"},
+                    {"name": "population", "type": "integer"},
+                ]
+            },
+            "expected_rows": [
+                {"id": 1, "name": "germany", "population": 83},
+                {"id": 2, "name": "france", "population": 66},
+                {"id": 3, "name": "spain", "population": 47},
+            ],
+        },
+        {
+            "test_id": "reverse_field_order",
+            "csv_path": "data/transform.csv",
+            "names": ["population", "name", "id"],
+            "expected_schema": {
+                "fields": [
+                    {"name": "population", "type": "integer"},
+                    {"name": "name", "type": "string"},
+                    {"name": "id", "type": "integer"},
+                ]
+            },
+            "expected_rows": [
+                {"population": 83, "name": "germany", "id": 1},
+                {"population": 66, "name": "france", "id": 2},
+                {"population": 47, "name": "spain", "id": 3},
+            ],
+        },
+        {
+            "test_id": "remove_last_column",
+            "csv_path": "data/transform.csv",
+            "names": ["id", "name"],
+            "expected_schema": {
+                "fields": [
+                    {"name": "id", "type": "integer"},
+                    {"name": "name", "type": "string"},
+                ]
+            },
+            "expected_rows": [
+                {"id": 1, "name": "germany"},
+                {"id": 2, "name": "france"},
+                {"id": 3, "name": "spain"},
+            ],
+        },
+        {
+            "test_id": "changed_field_order",
+            "csv_path": "data/transform.csv",
+            "names": ["name", "id"],
+            "expected_schema": {
+                "fields": [
+                    {"name": "name", "type": "string"},
+                    {"name": "id", "type": "integer"},
+                ]
+            },
+            "expected_rows": [
+                {"name": "germany", "id": 1},
+                {"name": "france", "id": 2},
+                {"name": "spain", "id": 3},
+            ],
+        },
+        {
+            "test_id": "missing_name",
+            "csv_path": "data/transform.csv",
+            "names": ["name", "nonexistent"],
+            "expected_schema": {
+                "fields": [
+                    {"name": "name", "type": "string"},
+                ]
+            },
+            "expected_rows": [
+                {"name": "germany"},
+                {"name": "france"},
+                {"name": "spain"},
+            ],
+        },
+        {
+            "test_id": "no_fields",
+            "csv_path": "data/transform.csv",
+            "names": [],
+            "expected_schema": {"fields": []},
+            "expected_rows": [
+                {},
+                {},
+                {},
+            ],
+        },
+        {
+            "test_id": "all_names_not_existing",
+            "csv_path": "data/transform.csv",
+            "names": ["extra_id", "extra_name", "extra_population"],
+            "expected_schema": {"fields": []},
+            "expected_rows": [
+                {},
+                {},
+                {},
+            ],
+        },
+    ],
+    ids=lambda scenario: scenario["test_id"],
+)
+def test_step_field_filter(scenario):
+    csv_path = scenario["csv_path"]
+    names = scenario["names"]
+    expected_schema = scenario["expected_schema"]
+    expected_rows = scenario["expected_rows"]
+
+    source = Resource(path=csv_path)
     target = transform(
         source,
         steps=[
-            steps.field_filter(names=["id", "name"]),
+            steps.field_filter(names=names),
         ],
     )
-    assert target.schema == {
-        "fields": [
-            {"name": "id", "type": "integer"},
-            {"name": "name", "type": "string"},
-        ]
-    }
-    assert target.read_rows() == [
-        {"id": 1, "name": "germany"},
-        {"id": 2, "name": "france"},
-        {"id": 3, "name": "spain"},
-    ]
+    assert target.schema == expected_schema
+    assert target.read_rows() == expected_rows